🧪 Add comprehensive tests for idstack-migrate#53
Conversation
Moved inline migration tests from smoke-test.sh to a dedicated test-migrate.sh script. Added new coverage for missing manifests and malformed json inputs. Co-authored-by: savvides <1580637+savvides@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the migration tests by moving them from test/smoke-test.sh into a dedicated unit test script test/test-migrate.sh. The feedback highlights potential issues in test/test-migrate.sh where the script might exit prematurely under set -e when testing failure scenarios, and suggests safely capturing exit codes. Additionally, it is recommended to run the migration tests directly in test/smoke-test.sh rather than conditionally checking for the script's existence, to avoid silently skipping tests if the file is missing or not executable.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| "$MIGRATE" "$WORK/project.json" >/dev/null 2>&1 | ||
| EC=$? | ||
| assert "missing manifest exits 0" "[ $EC -eq 0 ]" |
There was a problem hiding this comment.
Under set -e, if the $MIGRATE command fails, the script will immediately terminate at line 70 instead of continuing to the assertion. To safely capture the exit code without triggering set -e on failure, initialize EC and use the || operator.
| "$MIGRATE" "$WORK/project.json" >/dev/null 2>&1 | |
| EC=$? | |
| assert "missing manifest exits 0" "[ $EC -eq 0 ]" | |
| EC=0 | |
| "$MIGRATE" "$WORK/project.json" >/dev/null 2>&1 || EC=$? | |
| assert "missing manifest exits 0" "[ $EC -eq 0 ]" |
| OUT=$("$MIGRATE" "$WORK/project.json" 2>/dev/null) | ||
| EC=$? | ||
| assert "malformed manifest exits 0" "[ $EC -eq 0 ]" |
There was a problem hiding this comment.
If $MIGRATE fails here, the command substitution inside the variable assignment will trigger set -e and terminate the script immediately. Initialize EC and use || to safely capture the exit code.
| OUT=$("$MIGRATE" "$WORK/project.json" 2>/dev/null) | |
| EC=$? | |
| assert "malformed manifest exits 0" "[ $EC -eq 0 ]" | |
| EC=0 | |
| OUT=$("$MIGRATE" "$WORK/project.json" 2>/dev/null) || EC=$? | |
| assert "malformed manifest exits 0" "[ $EC -eq 0 ]" |
| if [ -x "$IDSTACK_DIR/test/test-migrate.sh" ]; then | ||
| check "idstack-migrate unit tests pass" "'$IDSTACK_DIR/test/test-migrate.sh'" | ||
| fi |
There was a problem hiding this comment.
If test-migrate.sh is missing or not executable, the smoke test will silently skip these unit tests instead of failing. It is safer to run the test script directly so that any execution failure or missing file is caught.
| if [ -x "$IDSTACK_DIR/test/test-migrate.sh" ]; then | |
| check "idstack-migrate unit tests pass" "'$IDSTACK_DIR/test/test-migrate.sh'" | |
| fi | |
| check "idstack-migrate unit tests pass" "'$IDSTACK_DIR/test/test-migrate.sh'" |
1 participant
🎯 What: The testing gap addressed
Created dedicated unit tests for the
idstack-migratebin script. Previously, these were just a block of inline checks insidesmoke-test.sh. The new test suite properly tests failure conditions and edge cases rather than only the happy paths.📊 Coverage: What scenarios are now tested
✨ Result: The improvement in test coverage
The code is now more robustly tested with 12 distinct assertions specifically targeting the migration logic, making the
smoke-test.shscript more maintainable and increasing confidence in schema changes.PR created automatically by Jules for task 14472023552349704849 started by @savvides