fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op - #84
Conversation
…as a no-op
tests/e2e/template_instantiation_test.sh ran:
find ... -exec bash -c '
file="$1"
... grep/sed over $file ...
' _ "$file"
Two defects in that one line:
1. No ';' or '+' terminator, so the file does not parse (SC2067).
2. "$file" is passed where {} belongs. $file is assigned ONLY inside the
-exec body, so in the outer scope it is UNSET — $1 arrived empty, file=""
and every grep/sed operated on an empty path.
⚠ The consequence is worse than a lint error: the placeholder-replacement step
SILENTLY DID NOTHING, then logged "All placeholder tokens replaced". A test
whose whole purpose is to prove instantiation worked was passing without
replacing a single token. That is a plausible cause of estate repos shipping
with literal {{project}} tokens still in their sources.
Corrected to "' _ {} \;" so find passes each matched path.
Found by an estate-wide shellcheck sweep of 5,111 scripts across 375 repos:
this identical stale copy exists in 30 repositories. rsr-template-repo's own
copy is already correct and restructured (371 lines vs the 268 here), so these
are stale duplicates that never picked up the upstream fix.
|
Warning Review limit reachedNext included review available in 42 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (13)
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe end-to-end template test now passes each ChangesTemplate instantiation test
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This localized test-script change fixes the find placeholder invocation without introducing a broader product or production behavior change; no actionable merge-blocking risk remains after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description clearly explains the defects and the intended fix, but it does not follow the repository template. It omits the required Summary, Changes, RSR Quality Checklist, Testing, and Screenshots sections. Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Up to standards ✅🟢 Issues
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
The PR correctly addresses a critical syntax error in the tests/e2e/template_instantiation_test.sh script. By terminating the find -exec command and properly passing the {} placeholder, it ensures that template files are actually processed rather than skipped. Codacy reports that the PR is up to standards with no new automated issues.
However, a cross-correlation of findings reveals a potential implementation gap. While the command syntax is fixed, several variables intended for template substitution (such as TEST_OWNER and TEST_FORGE) remain unused in the script. This suggests that the substitution logic may still be incomplete or relying on hardcoded values. Additionally, a minor performance optimization for the find command is recommended to handle multiple files more efficiently.
About this PR
- There are several test configuration variables (e.g., TEST_OWNER, TEST_FORGE, TEST_AUTHOR_EMAIL) assigned at the start of the script that are never utilized. This suggests that the
sedsubstitution logic within thefindcommand might be incomplete or using hardcoded strings instead of these dynamic values. Please verify if these variables should be integrated into the file processing loop.
1 comment outside of the diff
tests/e2e/template_instantiation_test.sh
line 21-32🟡 MEDIUM RISK
Several test configuration variables are assigned but never used. Verify if these should be incorporated into the replacement logic inside thefind -execloop. If they are redundant, they should be removed to simplify the script.
Test suggestions
- Verify that the find command correctly identifies template files and passes their paths to the sed replacement logic.
- Ensure the script does not produce SC2067 (missing terminator) linting errors.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
Co-authored-by: codacy-production[bot] <61871480+codacy-production[bot]@users.noreply.github.com> Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com>
|
1 participant
tests/e2e/template_instantiation_test.shranfind … -exec bash -c '…' _ "\$file", which has two defects on one line:;or+terminator — the file does not parse (SC2067)."\$file"where{}belongs —\$fileis assigned only inside the-execbody, so in the outer scope it is unset.\$1arrived empty,file="", and everygrep/sedoperated on an empty path.⚠ The consequence is worse than a lint error. The placeholder-replacement step silently did nothing, then logged "All placeholder tokens replaced". A test whose entire purpose is to prove instantiation worked was passing without replacing a single token — a plausible cause of estate repos shipping with literal
{{project}}still in their sources.Corrected to
' _ {} \;sofindpasses each matched path.Found by an estate-wide sweep of 5,111 scripts across 375 repos: this identical stale copy exists in 30 repositories.
rsr-template-repo's own copy is already correct and restructured (371 lines vs the 268 here), so these are stale duplicates that never picked up the upstream fix.