-
Notifications
You must be signed in to change notification settings - Fork 35
test(analysis): add tests for evolving the state over time #2180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Reviewer's GuideThis PR adds a temporal test suite for how latest-filter analysis results evolve over time, introduces test utilities for building path iterators and escaping query parameters, generalizes the document ingestion helper to accept any AsRef, and refactors existing tests to use the new Join-based path construction instead of hard-coded string slices. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- The new
Jointrait’schainmethod name is very close toIterator::chainbut has different semantics and is implemented for allDisplay; consider using a more specific name and/or a narrower impl to avoid confusion and unintended method resolution. - The temporal test cases include a
// FIXMEabout the expectedtotalfor the latest purl query; please either resolve the expectation or mark the test with an explicit reason (e.g.#[ignore]) so it doesn’t land in a known-broken state. - The new path-building via
Join+join/chainin tests is harder to read than the original literal string arrays; consider simplifying the helpers or keeping the test data paths as explicit lists where possible to preserve clarity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `Join` trait’s `chain` method name is very close to `Iterator::chain` but has different semantics and is implemented for all `Display`; consider using a more specific name and/or a narrower impl to avoid confusion and unintended method resolution.
- The temporal test cases include a `// FIXME` about the expected `total` for the latest purl query; please either resolve the expectation or mark the test with an explicit reason (e.g. `#[ignore]`) so it doesn’t land in a known-broken state.
- The new path-building via `Join` + `join`/`chain` in tests is harder to read than the original literal string arrays; consider simplifying the helpers or keeping the test data paths as explicit lists where possible to preserve clarity.
## Individual Comments
### Comment 1
<location> `modules/analysis/src/endpoints/tests/temporal.rs:68` </location>
<code_context>
+)]
+#[case( // purl search, latest
+ Req { what: What::Q(&format!("purl~{}", escape_q("pkg:oci/virt-handler-rhel9@sha256%3A507d126fa23811854bb17531194f9e832167022a1a30542561aadfd668bf1542?arch=amd64&os=linux&repository_url=registry.access.redhat.com%2Fcontainer-native-virtualization%2Fvirt-handler-rhel9&tag=v4.17.36-3"))), latest: true, ..Req::default() },
+ all(), 1 // FIXME: I'm not sure what to expect here, we had three matches, from three layers and now only one.
+)]
+#[test_log::test(actix_web::test)]
</code_context>
<issue_to_address>
**issue (testing):** Clarify and codify the expected behavior instead of keeping a FIXME in an assertion
The test currently asserts `total == 1` while a `FIXME` says the correct behavior (one match vs three layer matches) is unknown. This contradiction makes the test fragile and confusing. Please either define and document the intended semantics here and remove the FIXME, or refactor into more specific tests (e.g., per-layer vs latest-only behavior) so the expectations are explicit.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| )] | ||
| #[case( // purl search, latest | ||
| Req { what: What::Q(&format!("purl~{}", escape_q("pkg:oci/virt-handler-rhel9@sha256%3A507d126fa23811854bb17531194f9e832167022a1a30542561aadfd668bf1542?arch=amd64&os=linux&repository_url=registry.access.redhat.com%2Fcontainer-native-virtualization%2Fvirt-handler-rhel9&tag=v4.17.36-3"))), latest: true, ..Req::default() }, | ||
| all(), 1 // FIXME: I'm not sure what to expect here, we had three matches, from three layers and now only one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (testing): Clarify and codify the expected behavior instead of keeping a FIXME in an assertion
The test currently asserts total == 1 while a FIXME says the correct behavior (one match vs three layer matches) is unknown. This contradiction makes the test fragile and confusing. Please either define and document the intended semantics here and remove the FIXME, or refactor into more specific tests (e.g., per-layer vs latest-only behavior) so the expectations are explicit.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2180 +/- ##
=======================================
Coverage ? 68.24%
=======================================
Files ? 376
Lines ? 21205
Branches ? 21205
=======================================
Hits ? 14472
Misses ? 5863
Partials ? 870 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Assisted-by: Cursor
86e4f65 to
b2f5439
Compare
Assisted-by: Cursor
Summary by Sourcery
Add temporal analysis tests and shared helpers for constructing test document paths and escaped query parameters.
Enhancements:
Tests: