fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op - #41
fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op#41hyperpolymath wants to merge 1 commit into
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.
|
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. (2)
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe end-to-end template instantiation test now passes each matched file path from ChangesTemplate path handling
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This is a localized test-script correction with no actionable merge-blocking risk remaining beyond normal checks and review. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the defect, its impact, and the correction. However, it does not follow the repository template and 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
This PR addresses the immediate syntax errors in the find -exec command (missing terminator and placeholder), but the logic for template substitution remains a no-op.
A critical issue was identified where the use of single quotes for the sh -c subshell prevents the shell from expanding the $placeholder, $value, and $file variables. Consequently, the sed and grep operations will not perform any actual substitutions.
Additionally, several variables intended for template metadata (like TEST_OWNER and TEST_AUTHOR_EMAIL) are defined but unused, which correlates with the lack of verification in the current test suite. These issues should be resolved to ensure the E2E test provides meaningful coverage.
About this PR
- The PR fixes the command execution syntax but does not yet fulfill the intent of the E2E test. There is currently no verification logic to ensure that placeholders (e.g.,
{{project}}) are actually replaced with the expected values in the output files. Please ensure the test asserts the content of the instantiated templates.
1 comment outside of the diff
tests/e2e/template_instantiation_test.sh
line 21-32198🟡 MEDIUM RISK
Several test configuration variables (TEST_OWNER, TEST_FORGE, TEST_AUTHOR_EMAIL, TEST_DESCRIPTION, TEST_PRIMARY_LANGUAGE) and the YELLOW color constant are defined but never used. These should likely be used as the values for template substitution or within the test reporting logic.
Test suggestions
- Verify that template placeholders (e.g., {{project}}) are actually replaced with provided values in the instantiated files.
- Ensure the find command correctly identifies all files requiring substitution within the target directory.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that template placeholders (e.g., {{project}}) are actually replaced with provided values in the instantiated files.
2. Ensure the find command correctly identifies all files requiring substitution within the target directory.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| fi | ||
| done | ||
| ' _ "$file" | ||
| ' _ {} \; |
There was a problem hiding this comment.
🔴 HIGH RISK
The variables $placeholder, $value, and $file used in the sh -c block will not be expanded because the command string is single-quoted. Additionally, the file found by find is passed as the first argument ($1); you must either assign it (e.g., file="$1") or use $1 directly in the sed command.
Try running the following prompt in your coding agent:
In
tests/e2e/template_instantiation_test.sh, fix thefind -execcommand so that shell variables for placeholders and values are correctly passed into the subshell (e.g., as arguments$2,$3) and the file path from{}is correctly mapped to the variable used in thesedcommand.
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.