Skip to content

Conversation

@Synicix
Copy link
Contributor

@Synicix Synicix commented Aug 29, 2025

Depends on #111

After taking a look at using cargo features to allow testing. I believe this strays too far from the standard way Rust expects us to test things. In short I am splitting them based on the purpose of the test.

Resolves PIPE-118

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@Synicix Synicix requested review from Copilot and eywalker August 29, 2025 09:15
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the test organization to follow Rust testing standards by splitting tests into unit tests (within modules using #[cfg(test)]) and integration tests (in the tests/ directory). The changes move tests from the top-level tests/ directory that were testing internal implementation details into unit test modules within the source code files, while keeping true integration tests that verify public API usage in the tests/ directory.

  • Moved internal API tests (crypto, operator) from integration tests to unit tests within source modules
  • Updated test imports and API usage to reflect restructured modules and updated field names
  • Added new pipeline functionality with proper test coverage and data fixtures

Reviewed Changes

Copilot reviewed 43 out of 44 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
tests/store.rs Updated imports and refactored pod annotation test with cleaner Pod construction
tests/pipeline_runner.rs New integration test for pipeline runner functionality
tests/pipeline.rs Updated API calls to use new field names (NodeURI, pod instead of ref)
tests/orchestrator.rs Fixed iteration and filtering logic for container operations
tests/operator.rs Removed - moved to unit tests in src/core/operator.rs
tests/model.rs Updated hash values and test assertions
tests/fixture/mod.rs Added extensive pipeline test fixtures and helper functions
tests/crypto.rs Removed - moved to unit tests in src/core/crypto.rs
tests/agent.rs Updated metadata extraction to use manual parsing instead of removed utility
src/core/operator.rs Added unit tests moved from integration tests
src/core/crypto.rs Added unit tests moved from integration tests
Comments suppressed due to low confidence (1)

src/core/pipeline_runner.rs:1

  • Unnecessary double vector wrapping. Should be chain(std::iter::once(vec![packet.clone()])) for better performance and clarity.
use crate::{

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +102 to +109
let metadata: HashMap<String, String> = sample
.key_expr()
.as_str()
.split('/')
.map(ToOwned::to_owned)
.tuples()
.collect();
let topic_kind = metadata["event"].as_str();
Copy link

Copilot AI Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The metadata parsing logic is incorrect. This creates key-value pairs from consecutive elements, but the key expression format requires proper parsing of the structured path segments.

Suggested change
let metadata: HashMap<String, String> = sample
.key_expr()
.as_str()
.split('/')
.map(ToOwned::to_owned)
.tuples()
.collect();
let topic_kind = metadata["event"].as_str();
// Properly parse the key expression to extract the event kind
let key_segments: Vec<&str> = sample
.key_expr()
.as_str()
.split('/')
.collect();
// Find the segment after "pod_job" (which should be the event kind)
let topic_kind = key_segments
.iter()
.position(|&seg| seg == "pod_job")
.and_then(|idx| key_segments.get(idx + 1))
.map(|s| *s)
.unwrap_or("");

Copilot uses AI. Check for mistakes.
@codecov
Copy link

codecov bot commented Aug 29, 2025

@eywalker
Copy link
Contributor

Have reviewed through in-person discussion. This, however, warrants a more thorough future visit upon completely cleaning up the series of PRs.

@eywalker eywalker merged commit 49a79b0 into nauticalab:dev Oct 20, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Split test into integration test (test folder) vs unit testing

2 participants