fix: update test scripts to run all tests and handle remote branch conflicts#16
fix: update test scripts to run all tests and handle remote branch conflicts#16yellow-seed wants to merge 1 commit intomainfrom
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe pull request enhances CI/CD and testing infrastructure by modifying the test execution workflow to run all BAT tests, adding remote branch conflict resolution to the PR creation script, and introducing a comprehensive test suite for the PR creation workflow with extensive documentation updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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.
Pull request overview
This PR enhances the testing infrastructure and adds remote branch conflict handling to the PR creation workflow. The changes ensure all test files are executed during CI runs and prevent push failures when remote branches already exist from previous workflow runs.
Key changes:
- Added comprehensive test coverage for the PR creation script
- Implemented automatic deletion of existing remote branches before pushing
- Updated test execution commands to run all test files instead of a single file
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/create-update-pr.bats | New test file with 23 test cases covering PR creation, git operations, remote branch handling, and artifact validation |
| tests/README.md | Updated documentation to reference new test file and changed commands to run all tests |
| scripts/create-update-pr.sh | Added logic to detect and delete existing remote branches before pushing to prevent conflicts |
| .github/workflows/test-scripts.yml | Changed test execution to run all test files in the tests directory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Verify message contains key components | ||
| [[ "$expected_message" == *"Update Mozc to version 2.32.5994"* ]] | ||
| [[ "$expected_message" == *"ARM64 (Apple Silicon) only support"* ]] | ||
| [[ "$expected_message" == *"google/mozc commit abc123def456"* ]] |
There was a problem hiding this comment.
The expected_message variable contains a multi-line string with list items using hyphens, but the assertions only verify partial matches. Consider using a more robust comparison that validates the complete structure or individual lines to ensure the commit message format is exactly as expected.
| # Verify message contains key components | |
| [[ "$expected_message" == *"Update Mozc to version 2.32.5994"* ]] | |
| [[ "$expected_message" == *"ARM64 (Apple Silicon) only support"* ]] | |
| [[ "$expected_message" == *"google/mozc commit abc123def456"* ]] | |
| full_expected_message="Update Mozc to version $VERSION | |
| - Updated cask formula with version $VERSION | |
| - ARM64 (Apple Silicon) only support | |
| - Source: google/mozc commit $COMMIT_SHA | |
| - Artifacts from google/mozc GitHub Actions build" | |
| # Verify the complete message structure matches exactly | |
| [ "$expected_message" = "$full_expected_message" ] |
| # Verify PR body contains key information | ||
| [[ "$pr_body" == *"Update Mozc to version 2.32.5994"* ]] | ||
| [[ "$pr_body" == *"ARM64 (Apple Silicon) only support"* ]] | ||
| [[ "$pr_body" == *"https://github.com/google/mozc/commit/abc123def456"* ]] | ||
| [[ "$pr_body" == *"v2.32.5994"* ]] |
There was a problem hiding this comment.
Similar to the commit message test, this PR body validation only checks for partial string matches rather than verifying the complete structure. Consider adding assertions for specific sections (Changes, Artifacts, Source) to ensure the full template is properly formatted.
| # Verify PR body contains key information | |
| [[ "$pr_body" == *"Update Mozc to version 2.32.5994"* ]] | |
| [[ "$pr_body" == *"ARM64 (Apple Silicon) only support"* ]] | |
| [[ "$pr_body" == *"https://github.com/google/mozc/commit/abc123def456"* ]] | |
| [[ "$pr_body" == *"v2.32.5994"* ]] | |
| # Verify PR body contains structured sections | |
| changes_section=$'### Changes\n- Updated version to '"$VERSION"$'\n- ARM64 (Apple Silicon) only support\n- Included build artifacts from google/mozc commit ['"$COMMIT_SHA"$'](https://github.com/google/mozc/commit/'"$COMMIT_SHA"$')\n' | |
| artifacts_section=$'### Artifacts\n- `Mozc_arm64.pkg` (ARM64/Apple Silicon)\n' | |
| source_section=$'### Source\nBuilt from google/mozc GitHub Actions: https://github.com/google/mozc/commit/'"$COMMIT_SHA"$'\n' | |
| [[ "$pr_body" == *"$changes_section"* ]] | |
| [[ "$pr_body" == *"$artifacts_section"* ]] | |
| [[ "$pr_body" == *"$source_section"* ]] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/create-update-pr.bats (2)
184-187: Consider more robust commit message validation.The test uses partial string matching rather than validating the complete structure or individual lines. This approach was flagged in previous reviews as potentially missing format issues.
As suggested in previous reviews, consider validating the complete message structure or verifying each line individually to ensure the commit message format is exactly as expected.
318-322: Consider more robust PR body validation.Similar to the commit message test, this validation only checks for partial string matches rather than verifying the complete structure. Previous reviews suggested validating specific sections to ensure the full template is properly formatted.
As noted in previous reviews, consider adding assertions for specific sections (Changes, Artifacts, Source) with their complete expected content to ensure the PR body template is correctly formatted.
🧹 Nitpick comments (3)
tests/create-update-pr.bats (3)
1-22: Consider enhancing file header documentation.Per coding guidelines, the file header could include more comprehensive documentation:
- Overview of testing approach (mock file system for remote branch simulation)
- Dependencies (bats, git)
- Test environment setup details
🔎 Example enhanced header
#!/usr/bin/env bats -# Tests for create-update-pr.sh -# This test file validates the PR creation and git operations logic +# ========== Test Suite: create-update-pr.sh ========== +# +# Purpose: Validates PR creation workflow and git operations +# +# Overview: +# - Tests branch naming, git configuration, and artifact handling +# - Uses mock file system to simulate remote branch operations +# - Validates commit messages, PR body generation, and error handling +# +# Dependencies: +# - bats-core: Test framework +# - git: Version control operations +# +# Test Approach: +# - Creates temporary git repository for each test +# - Uses .remote_branches directory to mock remote state +# - Tests logic in isolation rather than full script integrationBased on coding guidelines for shell scripts.
31-54: Helper functions use mocking rather than actual git commands.The helper functions simulate remote branch operations using a mock file system (
.remote_branchesdirectory) rather than testing actualgit ls-remoteandgit push --deletecommands. This unit testing approach validates the logic in isolation but doesn't verify that the actual git commands work as expected.Consider adding integration tests that execute the actual
create-update-pr.shscript with a real remote repository (e.g., using a local bare git repository as a mock remote). This would catch issues like:
- Git command syntax errors
- Authentication/permission problems
- Network error handling
- Exit code handling from git commands
The current mock-based tests are still valuable for fast unit testing of logic, so both approaches could coexist.
Would you like me to generate an example integration test setup using a local bare repository as a mock remote?
56-323: Consider adding section headers for improved organization.Per coding guidelines for shell scripts, section headers like
# ========== Section Name ==========can improve readability by demarcating main code blocks. With 19 test cases, grouping them into logical sections would make the file easier to navigate.Example organization with section headers
# ========== Branch Management Tests ========== @test "generates correct branch name from version" { ... } @test "creates new branch correctly" { ... } @test "handles version strings with dots and numbers correctly" { ... } # ========== Remote Branch Conflict Resolution Tests ========== @test "detects when remote branch exists" { ... } @test "detects when remote branch does not exist" { ... } @test "deletes remote branch when it exists" { ... } @test "handles remote branch conflict by deleting existing branch" { ... } # ========== Git Configuration Tests ========== @test "configures git with bot identity correctly" { ... } # ========== Artifact Handling Tests ========== @test "validates artifacts directory structure" { ... } @test "stages correct files for commit" { ... } @test "calculates SHA256 checksum correctly" { ... } # ========== Message Generation Tests ========== @test "generates correct commit message format" { ... } @test "generates PR body with correct information" { ... } # ========== Cleanup and Navigation Tests ========== @test "removes temporary PR body file after use" { ... } @test "returns to original branch after operations" { ... } # ========== Error Handling Tests ========== @test "detects missing ARM64 package file" { ... }Based on coding guidelines for shell scripts.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/test-scripts.ymlscripts/create-update-pr.shtests/README.mdtests/create-update-pr.bats
🧰 Additional context used
📓 Path-based instructions (1)
**/*.sh
📄 CodeRabbit inference engine (AGENTS.md)
**/*.sh: Write appropriate granularity of explanatory comments in shell scripts to enhance code readability and maintainability
Include file header comments with script purpose, overview, usage instructions, and required environment variables/parameters
Use section headers like# ========== Section Name ==========to demarcate main processing stages and code blocks
Document shell functions with their purpose, arguments explanation, return values description, and usage examples for complex cases
Explain complex logic in shell scripts including condition branch intent, loop processing purpose, and regex/command pipeline operations
Document important configuration values and constants, explaining magic numbers and configuration choice rationale
Use#!/usr/bin/env bashshebang for improved portability across different systems
Implement appropriate error handling with error checks and messages for critical operations in shell scripts
Quote variables in shell scripts using double quotes (e.g.,"$variable") to prevent word splitting and globbing issues
Use functions in shell scripts to encapsulate repeated operations for improved reusability and maintainability
Files:
scripts/create-update-pr.sh
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: yellow-seed/brew_mozc PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:39:16.748Z
Learning: Follow a development workflow: confirm Issue understanding, create feature/fix branches, implement changes, run tests, create Pull Requests, and merge after review
🔇 Additional comments (3)
tests/README.md (2)
64-70: LGTM! Clear and helpful examples.The inline comments clarify the purpose of each command, and the examples properly demonstrate both running specific test files and all tests with verbose output.
88-103: LGTM! Comprehensive test coverage documentation.The new section thoroughly documents all test scenarios covered by create-update-pr.bats, following the same format as the existing test documentation. The inclusion of remote branch conflict handling in the coverage list is particularly helpful.
.github/workflows/test-scripts.yml (1)
55-55: LGTM! Proper test execution scope.Changing from running a single test file to running all tests in the directory aligns well with the new comprehensive test suite being added. This ensures all test coverage is executed in CI.
| # Check if remote branch exists and delete it to avoid conflicts | ||
| # This handles cases where a previous workflow run created the branch | ||
| if git ls-remote --exit-code --heads origin "$BRANCH_NAME" &> /dev/null; then | ||
| echo "Remote branch $BRANCH_NAME already exists, deleting it" | ||
| git push origin --delete "$BRANCH_NAME" || true | ||
| fi |
There was a problem hiding this comment.
Improve error handling for remote branch deletion.
The || true on line 171 suppresses all deletion failures, which means the script will continue even if the deletion fails due to authentication issues, permission problems, or network errors. When the subsequent push on line 175 fails with the same conflict, users won't understand why.
🔎 Proposed fix with explicit error handling
# Check if remote branch exists and delete it to avoid conflicts
# This handles cases where a previous workflow run created the branch
if git ls-remote --exit-code --heads origin "$BRANCH_NAME" &> /dev/null; then
echo "Remote branch $BRANCH_NAME already exists, deleting it"
- git push origin --delete "$BRANCH_NAME" || true
+ if ! git push origin --delete "$BRANCH_NAME"; then
+ echo "Warning: Failed to delete remote branch $BRANCH_NAME"
+ echo "The subsequent push may fail. Please check permissions and try again."
+ # Continue anyway in case the branch was deleted by another process
+ else
+ echo "Successfully deleted remote branch $BRANCH_NAME"
+ fi
fiAdditionally, consider whether automatically deleting remote branches is safe in all scenarios. If multiple workflows or developers might be working with the same branch name, this could cause unexpected data loss.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Check if remote branch exists and delete it to avoid conflicts | |
| # This handles cases where a previous workflow run created the branch | |
| if git ls-remote --exit-code --heads origin "$BRANCH_NAME" &> /dev/null; then | |
| echo "Remote branch $BRANCH_NAME already exists, deleting it" | |
| git push origin --delete "$BRANCH_NAME" || true | |
| fi | |
| # Check if remote branch exists and delete it to avoid conflicts | |
| # This handles cases where a previous workflow run created the branch | |
| if git ls-remote --exit-code --heads origin "$BRANCH_NAME" &> /dev/null; then | |
| echo "Remote branch $BRANCH_NAME already exists, deleting it" | |
| if ! git push origin --delete "$BRANCH_NAME"; then | |
| echo "Warning: Failed to delete remote branch $BRANCH_NAME" | |
| echo "The subsequent push may fail. Please check permissions and try again." | |
| # Continue anyway in case the branch was deleted by another process | |
| else | |
| echo "Successfully deleted remote branch $BRANCH_NAME" | |
| fi | |
| fi |
🤖 Prompt for AI Agents
In scripts/create-update-pr.sh around lines 167-172, the remote-branch deletion
step currently swallows all errors via "|| true"; change this so failures are
detected and surfaced: remove the "|| true", capture the git push exit code, and
if non-zero print a clear error message including the git output and exit
non-zero to fail the workflow; optionally gate automatic deletion behind an
explicit env var (e.g. FORCE_DELETE_REMOTE=true) to avoid unintentional data
loss in multi-developer scenarios and document that behavior.
1 participant
Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.