-
Notifications
You must be signed in to change notification settings - Fork 25
Avoid race condition on unit-tests #1277
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: master
Are you sure you want to change the base?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnsures deterministic behavior and avoids race-condition-related test failures when validating missing bundle images by ordering the computed diff set, and updates the related unit test expectation to match the new deterministic ordering. 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:
- Sorting and then immediately converting back to a set (
diff = set(sorted(diff))) does not make the error message deterministic, since sets are unordered; if you need stable ordering in the exception text, keepdiffas a list (e.g.,diff = sorted(diff)and interpolate that) instead of converting back to a set. - The updated test expectation reorders elements inside a
set(...), but set literals are inherently order-insensitive, so this change is effectively a no-op for the assertion; consider asserting on a list or the exact error message string if the goal is to verify deterministic ordering.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Sorting and then immediately converting back to a set (`diff = set(sorted(diff))`) does not make the error message deterministic, since sets are unordered; if you need stable ordering in the exception text, keep `diff` as a list (e.g., `diff = sorted(diff)` and interpolate that) instead of converting back to a set.
- The updated test expectation reorders elements inside a `set(...)`, but set literals are inherently order-insensitive, so this change is effectively a no-op for the assertion; consider asserting on a list or the exact error message string if the goal is to verify deterministic ordering.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
f7af642 to
6a17c0d
Compare
Use ordered diff for `get_bundles_latest_version` when raising to avoid race condition failures such as: https://github.com/release-engineering/iib/actions/runs/20110179089/job/57705081674?pr=1276 Signed-off-by: Jonathan Gangi <jgangi@redhat.com>
Use ordered diff for
get_bundles_latest_versionwhen raising to avoid race condition failures such as:https://github.com/release-engineering/iib/actions/runs/20110179089/job/57705081674?pr=1276
Summary by Sourcery
Ensure deterministic handling of missing bundle images when resolving latest bundle versions to avoid race-condition-related test failures.
Bug Fixes:
Tests: