fix: simplify cliproxyapi backup spec preprocessing - #457
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughMoves backup script preprocessing from per-test setup to describe-time initialization. Creates a temporary directory and preprocesses backup-auth.sh and backup-and-recover.sh by substituting placeholders and injecting processed script paths. Updates test invocations to reference the pre-generated scripts. Removes per-test preprocessing calls and adds cleanup to remove the temporary directory after all tests complete. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
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 |
Summary of ChangesHello @shunkakinoki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the testing strategy for Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Mesa DescriptionTL;DRSimplified What changed?File-level changes are not available. Description generated by Mesa. Update settings |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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 backup spec tests to preprocess scripts once at the beginning, which is a great improvement for efficiency. The changes look good overall. I've added a couple of suggestions to improve the maintainability and robustness of the new preprocessing and cleanup logic.
| cleanup_preprocessed() { | ||
| rm -rf "$__PREPROCESSED_DIR" | ||
| } |
There was a problem hiding this comment.
For improved safety, it's a good practice to add a check to ensure that __PREPROCESSED_DIR is a non-empty string and an existing directory before attempting to remove it recursively. This prevents accidental deletion of unintended files or directories if the variable were to be unexpectedly empty or unset.
| cleanup_preprocessed() { | |
| rm -rf "$__PREPROCESSED_DIR" | |
| } | |
| cleanup_preprocessed() { | |
| if [[ -n "$__PREPROCESSED_DIR" && -d "$__PREPROCESSED_DIR" ]]; then | |
| rm -rf "$__PREPROCESSED_DIR" | |
| fi | |
| } |
| __PREPROCESSED_DIR=$(mktemp -d) | ||
| __BACKUP_AUTH_SCRIPT="$__PREPROCESSED_DIR/backup-auth.sh" | ||
| __BACKUP_RECOVER_SCRIPT="$__PREPROCESSED_DIR/backup-and-recover.sh" | ||
|
|
||
| # Preprocess backup-auth.sh | ||
| sed \ | ||
| -e 's|@aws@|aws|g' \ | ||
| -e 's|@rsync@|rsync|g' \ | ||
| -e 's|@bash@|bash|g' \ | ||
| -e 's|@sed@|sed|g' \ | ||
| "$SCRIPTS_DIR/backup-auth.sh" >"$__BACKUP_AUTH_SCRIPT" | ||
| chmod +x "$__BACKUP_AUTH_SCRIPT" | ||
|
|
||
| # Preprocess backup-and-recover.sh with path to preprocessed backup-auth.sh | ||
| sed \ | ||
| -e 's|@aws@|aws|g' \ | ||
| -e 's|@rsync@|rsync|g' \ | ||
| -e 's|@bash@|bash|g' \ | ||
| -e 's|@sed@|sed|g' \ | ||
| -e "s|@backupAuthScript@|$__BACKUP_AUTH_SCRIPT|g" \ | ||
| "$SCRIPTS_DIR/backup-and-recover.sh" >"$__BACKUP_RECOVER_SCRIPT" | ||
| chmod +x "$__BACKUP_RECOVER_SCRIPT" |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication, you can define the common sed arguments in an array and reuse it. Also, consider using mktemp -d -t <template> to create temporary directories with a more descriptive name, which can aid in debugging.
| __PREPROCESSED_DIR=$(mktemp -d) | |
| __BACKUP_AUTH_SCRIPT="$__PREPROCESSED_DIR/backup-auth.sh" | |
| __BACKUP_RECOVER_SCRIPT="$__PREPROCESSED_DIR/backup-and-recover.sh" | |
| # Preprocess backup-auth.sh | |
| sed \ | |
| -e 's|@aws@|aws|g' \ | |
| -e 's|@rsync@|rsync|g' \ | |
| -e 's|@bash@|bash|g' \ | |
| -e 's|@sed@|sed|g' \ | |
| "$SCRIPTS_DIR/backup-auth.sh" >"$__BACKUP_AUTH_SCRIPT" | |
| chmod +x "$__BACKUP_AUTH_SCRIPT" | |
| # Preprocess backup-and-recover.sh with path to preprocessed backup-auth.sh | |
| sed \ | |
| -e 's|@aws@|aws|g' \ | |
| -e 's|@rsync@|rsync|g' \ | |
| -e 's|@bash@|bash|g' \ | |
| -e 's|@sed@|sed|g' \ | |
| -e "s|@backupAuthScript@|$__BACKUP_AUTH_SCRIPT|g" \ | |
| "$SCRIPTS_DIR/backup-and-recover.sh" >"$__BACKUP_RECOVER_SCRIPT" | |
| chmod +x "$__BACKUP_RECOVER_SCRIPT" | |
| __PREPROCESSED_DIR=$(mktemp -d -t cliproxy-spec-XXXXXXXX) | |
| __BACKUP_AUTH_SCRIPT="$__PREPROCESSED_DIR/backup-auth.sh" | |
| __BACKUP_RECOVER_SCRIPT="$__PREPROCESSED_DIR/backup-and-recover.sh" | |
| COMMON_SED_ARGS=( | |
| -e 's|@aws@|aws|g' | |
| -e 's|@rsync@|rsync|g' | |
| -e 's|@bash@|bash|g' | |
| -e 's|@sed@|sed|g' | |
| ) | |
| # Preprocess backup-auth.sh | |
| sed "${COMMON_SED_ARGS[@]}" "$SCRIPTS_DIR/backup-auth.sh" >"$__BACKUP_AUTH_SCRIPT" | |
| chmod +x "$__BACKUP_AUTH_SCRIPT" | |
| # Preprocess backup-and-recover.sh with path to preprocessed backup-auth.sh | |
| sed "${COMMON_SED_ARGS[@]}" \ | |
| -e "s|@backupAuthScript@|$__BACKUP_AUTH_SCRIPT|g" \ | |
| "$SCRIPTS_DIR/backup-and-recover.sh" >"$__BACKUP_RECOVER_SCRIPT" | |
| chmod +x "$__BACKUP_RECOVER_SCRIPT" |
There was a problem hiding this comment.
Pull request overview
This PR refactors the cliproxyapi_backup_spec.sh test file to preprocess script placeholders once at describe-time rather than during each test setup. This ensures preprocessed script paths are available before any tests run, eliminating the need for the nix_script_preprocess helper functions.
Key Changes:
- Moved script preprocessing from test setup to describe-time, creating preprocessed scripts once in a temporary directory
- Replaced
nix_script_preprocessandnix_script_preprocess_with_depscalls with direct sed commands - Added
AfterAllcleanup hook to remove the preprocessed directory after all tests complete
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Preprocess scripts once at describe-time (not in setup) | ||
| # This ensures the preprocessed paths are available before any tests run | ||
| __PREPROCESSED_DIR=$(mktemp -d) |
There was a problem hiding this comment.
The temporary directory created at describe-time is not cleaned up if the test suite is interrupted before reaching the AfterAll hook. This could lead to accumulating temporary directories in /tmp. Consider using a trap or ensuring the cleanup happens even on early exit or test failure.
| __PREPROCESSED_DIR=$(mktemp -d) | |
| __PREPROCESSED_DIR=$(mktemp -d) | |
| __cleanup_preprocessed_dir() { | |
| if [ -n "$__PREPROCESSED_DIR" ] && [ -d "$__PREPROCESSED_DIR" ]; then | |
| rm -rf "$__PREPROCESSED_DIR" | |
| fi | |
| } | |
| # Ensure the preprocessed directory is cleaned up even if the test suite is interrupted | |
| trap '__cleanup_preprocessed_dir' EXIT INT TERM |
| -e 's|@aws@|aws|g' \ | ||
| -e 's|@rsync@|rsync|g' \ | ||
| -e 's|@bash@|bash|g' \ | ||
| -e 's|@sed@|sed|g' \ |
There was a problem hiding this comment.
The sed replacement for @Sed@ placeholder is unnecessary here because the backup-auth.sh script does not use @Sed@. Looking at the actual script content, only @aws@ and @rsync@ placeholders are present in backup-auth.sh. Including unnecessary replacements can be confusing and may mask issues if the wrong script is accidentally preprocessed.
| -e 's|@sed@|sed|g' \ |
| -e 's|@aws@|aws|g' \ | ||
| -e 's|@rsync@|rsync|g' \ | ||
| -e 's|@bash@|bash|g' \ | ||
| -e 's|@sed@|sed|g' \ |
There was a problem hiding this comment.
The sed replacements for @aws@, @rsync@, and @Sed@ are unnecessary for backup-and-recover.sh. Looking at the actual script content, it only uses @bash@ and @backupAuthScript@ placeholders. Including unnecessary replacements makes the preprocessing logic harder to maintain and understand.
| -e 's|@aws@|aws|g' \ | |
| -e 's|@rsync@|rsync|g' \ | |
| -e 's|@bash@|bash|g' \ | |
| -e 's|@sed@|sed|g' \ | |
| -e 's|@bash@|bash|g' \ |
| sed \ | ||
| -e 's|@aws@|aws|g' \ | ||
| -e 's|@rsync@|rsync|g' \ | ||
| -e 's|@bash@|bash|g' \ | ||
| -e 's|@sed@|sed|g' \ | ||
| "$SCRIPTS_DIR/backup-auth.sh" >"$__BACKUP_AUTH_SCRIPT" | ||
| chmod +x "$__BACKUP_AUTH_SCRIPT" | ||
|
|
||
| # Preprocess backup-and-recover.sh with path to preprocessed backup-auth.sh | ||
| sed \ | ||
| -e 's|@aws@|aws|g' \ | ||
| -e 's|@rsync@|rsync|g' \ | ||
| -e 's|@bash@|bash|g' \ | ||
| -e 's|@sed@|sed|g' \ | ||
| -e "s|@backupAuthScript@|$__BACKUP_AUTH_SCRIPT|g" \ | ||
| "$SCRIPTS_DIR/backup-and-recover.sh" >"$__BACKUP_RECOVER_SCRIPT" | ||
| chmod +x "$__BACKUP_RECOVER_SCRIPT" |
There was a problem hiding this comment.
The preprocessing sed commands do not check for errors. If the source script files don't exist or if writing to the output fails, the test will continue with potentially empty or corrupted script files, leading to confusing test failures. Consider adding error checking after these preprocessing operations.
| sed \ | |
| -e 's|@aws@|aws|g' \ | |
| -e 's|@rsync@|rsync|g' \ | |
| -e 's|@bash@|bash|g' \ | |
| -e 's|@sed@|sed|g' \ | |
| "$SCRIPTS_DIR/backup-auth.sh" >"$__BACKUP_AUTH_SCRIPT" | |
| chmod +x "$__BACKUP_AUTH_SCRIPT" | |
| # Preprocess backup-and-recover.sh with path to preprocessed backup-auth.sh | |
| sed \ | |
| -e 's|@aws@|aws|g' \ | |
| -e 's|@rsync@|rsync|g' \ | |
| -e 's|@bash@|bash|g' \ | |
| -e 's|@sed@|sed|g' \ | |
| -e "s|@backupAuthScript@|$__BACKUP_AUTH_SCRIPT|g" \ | |
| "$SCRIPTS_DIR/backup-and-recover.sh" >"$__BACKUP_RECOVER_SCRIPT" | |
| chmod +x "$__BACKUP_RECOVER_SCRIPT" | |
| if ! sed \ | |
| -e 's|@aws@|aws|g' \ | |
| -e 's|@rsync@|rsync|g' \ | |
| -e 's|@bash@|bash|g' \ | |
| -e 's|@sed@|sed|g' \ | |
| "$SCRIPTS_DIR/backup-auth.sh" >"$__BACKUP_AUTH_SCRIPT"; then | |
| echo "Error: failed to preprocess backup-auth.sh from $SCRIPTS_DIR" >&2 | |
| exit 1 | |
| fi | |
| if ! chmod +x "$__BACKUP_AUTH_SCRIPT"; then | |
| echo "Error: failed to make preprocessed backup-auth.sh executable at $__BACKUP_AUTH_SCRIPT" >&2 | |
| exit 1 | |
| fi | |
| # Preprocess backup-and-recover.sh with path to preprocessed backup-auth.sh | |
| if ! sed \ | |
| -e 's|@aws@|aws|g' \ | |
| -e 's|@rsync@|rsync|g' \ | |
| -e 's|@bash@|bash|g' \ | |
| -e 's|@sed@|sed|g' \ | |
| -e "s|@backupAuthScript@|$__BACKUP_AUTH_SCRIPT|g" \ | |
| "$SCRIPTS_DIR/backup-and-recover.sh" >"$__BACKUP_RECOVER_SCRIPT"; then | |
| echo "Error: failed to preprocess backup-and-recover.sh from $SCRIPTS_DIR" >&2 | |
| exit 1 | |
| fi | |
| if ! chmod +x "$__BACKUP_RECOVER_SCRIPT"; then | |
| echo "Error: failed to make preprocessed backup-and-recover.sh executable at $__BACKUP_RECOVER_SCRIPT" >&2 | |
| exit 1 | |
| fi |
Refactors the cliproxyapi backup spec to preprocess scripts once at describe-time instead of using nix_script_preprocess helpers. This ensures preprocessed paths are available before any tests run.
Note
Refactors the backup script specs to preprocess target scripts once per suite and run tests against those preprocessed paths.
backup-auth.shandbackup-and-recover.shat describe-time into a temp dir usingsed(replacing@aws@,@rsync@,@bash@,@sed@and wiring@backupAuthScript@), then marks them executablenix_script_preprocess/nix_script_preprocess_with_depsand related cleanup; updatesWhen run ...to invoke the preprocessed scripts directlyHOMEinline), maintains aws/rsync mocks, and addsAfterAllto delete the temp preprocessed directory.envsourcing + backup flowWritten by Cursor Bugbot for commit 2703814. Configure here.
Summary by cubic
Preprocess the cliproxyapi backup scripts once at describe-time so tests use stable, real paths before any test runs. Removes nix_script_preprocess helpers to cut setup overhead and reduce flakiness.
Written for commit 2703814. Summary will update automatically on new commits.