Add regression test for concat string through println then shell#3726
Add regression test for concat string through println then shell#3726ATX24 wants to merge 2 commits into
Conversation
Co-authored-by: Dhilan Shah <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a non-Windows Tokio test ChangesShell Test Addition
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
|
No description provided. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/baml_tests/tests/shell.rs`:
- Around line 50-51: The test shell_concat_command_survives_println_before_shell
is brittle on Windows because it asserts an exact "hello_world\n"; either gate
the test with #[cfg(not(target_os = "windows"))] to run only on non-Windows
platforms or normalize the captured stdout before asserting (trim trailing CR/LF
or replace "\r\n" with "\n") in the test function so the assertion is
platform‑agnostic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cf0a29f9-7fd0-4991-aaf7-35000e53f7f8
📒 Files selected for processing (1)
baml_language/crates/baml_tests/tests/shell.rs
Co-authored-by: Dhilan Shah <[email protected]>
The error
Reported failing BAML snippet:
I could not reproduce the reported silent-empty failure on the current compiler2 implementation. Before changing repository files, I ran the snippet through the
baml_testsVM harness from a disposable crate outside the repo. Its real output was:Root cause
No current compiler/runtime defect was observed in
baml_language/. The gap is test coverage:crates/baml_tests/tests/shell.rscovered dynamic shell command strings, but did not specifically lock down the sequence where a concat-built command is first consumed bybaml.io.printlnand then passed tobaml.sys.shell.The fix
Added a regression test in
baml_language/crates/baml_tests/tests/shell.rsthat executes BAML snippets covering:let cmd = "echo " + "hello_world"baml.io.println("-> " + cmd)beforebaml.sys.shell(cmd, null)letbindingsprintlnresult to_Each case asserts that shell stdout is not empty and exactly equals
"hello_world\n".Verification
Initial reproduction before repository changes:
Targeted regression test:
Workspace/test gates run from
baml_language/:Additional setup/checks needed for the full local run:
Snapshot status: no snapshot failures occurred during the full
cargo test --quietrun, so no snapshots were regenerated.CodeRabbit: not run because
CODERABBIT_API_KEYis missing from the environment; the CLI could not be authenticated non-interactively.Issue Reference
Changes
Added targeted compiler2 regression coverage for concat-built shell command strings after
baml.io.printlnconsumption.Testing
Screenshots
Not applicable.
PR Checklist
Additional Notes
No compiler/runtime fix was added because the reported failure did not reproduce on the current compiler2 code.
Summary by CodeRabbit