fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op - #94
fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op#94hyperpolymath wants to merge 3 commits 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. (37)
🔇 Additional comments (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe end-to-end template instantiation test now exports replacement variables and passes each matched file path from ChangesTemplate instantiation test
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This change makes the template-instantiation test correctly process matched files and replace placeholders; 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 defect, its impact, and the correction. However, it does not follow the repository template and omits the required Changes, RSR Quality Checklist, Testing, and Screenshots sections.
✨ 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 successfully identifies and addresses a syntax error in the find -exec command that rendered the template instantiation tests a no-op. While the fix correctly terminates the command and passes the file placeholder, it is currently insufficient for the logic to function as intended.
Codacy analysis indicates the PR is up to standards; however, findings suggest that the configuration variables (e.g., TEST_OWNER, TEST_AUTHOR_EMAIL) are not being exported to the subshell, meaning the sed replacements will likely use empty strings. Additionally, the repository is maintaining a significantly stale version of this test script compared to the authoritative upstream source, which may lead to further technical debt.
About this PR
- The file
tests/e2e/template_instantiation_test.shis a stale copy (268 lines) compared to the authoritative version in the template-repo (371 lines). While this fix corrects an immediate defect, the repository remains divergent from the upstream source. Consider syncing with the upstream version to ensure all latest test logic and improvements are included.
Test suggestions
- Verify that template placeholders (e.g., {{project}}) are successfully replaced with actual values in the instantiated output files.
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.
🟡 MEDIUM RISK
Suggestion: The fix to the find command ensures the subshell executes, but the placeholder replacement may still fail for two reasons:
- Variable Scoping: The variables like
TEST_OWNERandTEST_AUTHOR_EMAILare local to this script. To make them visible to thesh -csubshell, they must be exported. - Positional Parameters: Ensure the script inside
sh -cassigns$1(which receives the{}filename) to thefilevariable used in the replacement logic.
Try running the following prompt in your IDE agent:
Export the configuration variables (TEST_OWNER, TEST_FORGE, TEST_AUTHOR_EMAIL, TEST_DESCRIPTION, TEST_PRIMARY_LANGUAGE) and ensure the
find -execsubshell script maps the positional parameter$1to the variablefileused in thesedcommands.
See Issue in Codacy
See Issue in Codacy
See Issue in Codacy
See Issue in Codacy
See Issue in Codacy
|
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.