ci: harden release workflow and fix flaky test#102
Merged
Conversation
The attest job only checked release_created, so manual workflow_dispatch with force_release=true would build wheels then skip attestation and publishing. Now both attest gate and checkout ref respect the force_release/release_tag inputs.
- Add validate-inputs job that fails fast when force_release=true but release_tag is empty (prevents publishing from wrong commit) - Add explicit if-guard to publish job (defense in depth) - Wire validate-inputs into build-wheels and build-sdist needs
Use threading.Barrier for simultaneous thread launch and tighter parameters (capacity=5, 20 workers, work >> timeout) so rejections are guaranteed by construction, not by timing luck.
📝 WalkthroughWalkthroughA test for concurrent operations was refactored to improve determinism by adjusting controller configuration parameters, introducing thread synchronization via a barrier, and adding explicit error handling for barrier failures. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_load_control.py (1)
151-152: Reduce magic numbers to avoid future drift in this test.
20and2are duplicated across setup and assertions. If one value changes and another is missed, this test can become brittle or misleading.Suggested refactor
- controller = BackpressureController(max_concurrent=2, queue_size=3, timeout=0.01) + total_workers = 20 + controller = BackpressureController(max_concurrent=2, queue_size=3, timeout=0.01) results = [] exceptions = [] - barrier = threading.Barrier(20, timeout=5) + barrier = threading.Barrier(total_workers, timeout=5) threads = [] - for i in range(20): + for i in range(total_workers): t = threading.Thread(target=worker, args=(i,)) threads.append(t) t.start() @@ - assert len(results) + len(exceptions) == 20 + assert len(results) + len(exceptions) == total_workers @@ - assert controller._semaphore._value == 2 # Back to max_concurrent + assert controller._semaphore._value == controller.max_concurrentAlso applies to: 158-158, 174-174, 184-184, 188-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_load_control.py` around lines 151 - 152, Replace hardcoded numbers with named constants so the test doesn't drift: introduce local constants (e.g., total_threads, max_workers, queue_len) and use them when calling max_concurrent(max_workers) and queue_size(queue_len), compute capacity = max_concurrent(max_workers) + queue_size(queue_len), and use total_threads and capacity in the assertion that expects rejected == total_threads - capacity (and in any other places currently using 20 or 2). Update references around max_concurrent, queue_size, and the capacity calculation so the setup and assertions derive from the same constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/test_load_control.py`:
- Around line 151-152: Replace hardcoded numbers with named constants so the
test doesn't drift: introduce local constants (e.g., total_threads, max_workers,
queue_len) and use them when calling max_concurrent(max_workers) and
queue_size(queue_len), compute capacity = max_concurrent(max_workers) +
queue_size(queue_len), and use total_threads and capacity in the assertion that
expects rejected == total_threads - capacity (and in any other places currently
using 20 or 2). Update references around max_concurrent, queue_size, and the
capacity calculation so the setup and assertions derive from the same constants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be08f5fb-a16b-4c22-87c7-08f2e768382f
📒 Files selected for processing (1)
tests/unit/test_load_control.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
validate-inputsjob that rejectsforce_releasewithout a valid semver tag (vX.Y.Z) and verifies the tag exists on the remotegithub.event.inputs(prevents script injection)ifguard onpublishjob (defense in depth)force_releasegate throughattestandpublishjobs so manual re-releases complete end-to-endtest_concurrent_operations— usethreading.Barrierfor deterministic contention instead of timing-dependenttime.sleepTest plan
gh workflow run release-please.yml --ref main -f force_release=true -f release_tag=v0.6.0force_release=truewithoutrelease_tagfails fastSummary by CodeRabbit
Release Notes
This release contains internal test improvements with no user-facing changes. Test reliability enhancements were made to improve code quality assurance processes.