-
Notifications
You must be signed in to change notification settings - Fork 693
Fix CodexConfigHelperTests for new prerelease argument order #652
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
Conversation
…x command The uvx command now includes --prerelease and explicit arguments before --from due to beta server mode support. Updated test assertions to check for the correct argument order: Before: [--from, mcpforunityserver>=0.0.0a0, mcp-for-unity, ...] After: [--prerelease, explicit, --from, mcpforunityserver>=0.0.0a0, mcp-for-unity, ...] Updated tests: - BuildCodexServerBlock_OnWindows_IncludesSystemRootEnv - BuildCodexServerBlock_OnNonWindows_ExcludesEnv - UpsertCodexServerBlock_OnWindows_IncludesSystemRootEnv - UpsertCodexServerBlock_OnNonWindows_ExcludesEnv Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Reviewer's GuideAdjusts CodexConfigHelper tests to match the new uvx command argument order that now prepends File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughTest assertions were updated to expect an expanded five-argument structure for Codex server commands, replacing the previous three-argument validation. The pattern Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 - I've left some high level feedback:
- These tests are now quite brittle to exact argument ordering and count (e.g., indexing 0–4 with
ChildrenCount >= 5); consider asserting the presence and relative order of key arguments in a more flexible way (or usingChildrenCount == 5) to avoid breakage on benign changes to the CLI. - The argument validation logic is duplicated across the four test cases; extracting a shared helper to assert the uvx arg structure would reduce repetition and keep future changes to the arg layout in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- These tests are now quite brittle to exact argument ordering and count (e.g., indexing 0–4 with `ChildrenCount >= 5`); consider asserting the presence and relative order of key arguments in a more flexible way (or using `ChildrenCount == 5`) to avoid breakage on benign changes to the CLI.
- The argument validation logic is duplicated across the four test cases; extracting a shared helper to assert the uvx arg structure would reduce repetition and keep future changes to the arg layout in one place.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary
Updates CodexConfigHelperTests to account for the new
--prerelease explicitarguments added to the uvx command for beta server mode support.Changes
Updated assertion indices in 4 test methods to match the new argument order:
--prerelease(index 0)explicit(index 1)--from(index 2)mcpforunityserver>=0.0.0a0(index 3)mcp-for-unity(index 4)Previously the tests expected
--fromat index 0, but with beta server mode support, these arguments are now prepended.Tests Fixed
🤖 Generated with Claude Code
Summary by Sourcery
Tests:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.