Fix and unskip flakey integration test in replace.test.ts #11060
Conversation
|
Size Change: -2 B (0%) Total Size: 17.8 MB ℹ️ View Unchanged
|
89b2595 to
ec12446
Compare
|
/gemini review |
3fcbc10 to
c08a0b6
Compare
There was a problem hiding this comment.
Code Review
This pull request aims to fix a flaky integration test and clean up the test suite. The changes in replace.test.ts simplify the tests and make the previously flaky test more robust by providing a more constrained prompt to the model. The change in scripts/deflake.js is intended to improve the script's argument handling. My review focuses on potential issues in the new code. I've identified a potential source of flakiness in one of the simplified tests due to removed line-ending normalization, and a bug in the new argument handling logic in the deflake.js script that affects arguments with spaces.
09410a2 to
fe2b8da
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a flaky integration test in replace.test.ts by providing more explicit instructions to the AI model and streamlining the test code. These changes enhance the reliability and readability of the test suite. The deflake.js script is also refactored for better command-line argument handling. I've identified a critical security vulnerability in the updated deflake.js script and provided a suggestion to mitigate the risk of command injection.
596a1d5 to
0f5d74d
Compare
0f5d74d to
b3c9ad8
Compare
TLDR
Fixes "should fail safely when old_string is not found" to give the agent clearer directions.
Also: Fix arg parsing in deflake.ts so I can run:
instead of having to run everything.
Reviewer Test Plan
See this run using
deflake. There was one flake but not from this test file: https://github.com/google-gemini/gemini-cli/actions/runs/18480274254/job/52653514207?pr=11073Linked issues / bugs
Fixes #10916