-
Notifications
You must be signed in to change notification settings - Fork 6
Cod 1815 - Add the full command command that was run to the benchmark uri #191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cod 1815 - Add the full command command that was run to the benchmark uri #191
Conversation
not-matthias
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can improve the testing a bit, to avoid re-implementing the same set of shell tests in every new executor (lmk if there are any technical reasons that prevent this for the exec-harness).
Other than that, it looks pretty good. Next review should be quick :)
| } | ||
|
|
||
| /// Test that a command with shell operators (&&) works correctly when passed as a single argument | ||
| #[test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have the executor tests, which already include a lot of these edge cases:
Lines 14 to 50 in 0cfb188
| const TESTS: [&str; 6] = [ | |
| // Simple echo command | |
| "echo 'Hello, World!'", | |
| // Multi-line commands without semicolons | |
| "echo \"Working\" | |
| echo \"with\" | |
| echo \"multiple lines\"", | |
| // Multi-line commands with semicolons | |
| "echo \"Working\"; | |
| echo \"with\"; | |
| echo \"multiple lines\";", | |
| // Directory change and validation | |
| "cd /tmp | |
| # Check that the directory is actually changed | |
| if [ $(basename $(pwd)) != \"tmp\" ]; then | |
| exit 1 | |
| fi", | |
| // Quote escaping test | |
| "#!/bin/bash | |
| VALUE=\"He said \\\"Hello 'world'\\\" & echo \\$HOME\" | |
| if [ \"$VALUE\" = \"He said \\\"Hello 'world'\\\" & echo \\$HOME\" ]; then | |
| echo \"Quote test passed\" | |
| else | |
| echo \"ERROR: Quote handling failed\" | |
| exit 1 | |
| fi", | |
| // Command substitution test | |
| "#!/bin/bash | |
| RESULT=$(echo \"test 'nested' \\\"quotes\\\" here\") | |
| COUNT=$(echo \"$RESULT\" | wc -w) | |
| if [ \"$COUNT\" -eq \"4\" ]; then | |
| echo \"Command substitution test passed\" | |
| else | |
| echo \"ERROR: Expected 4 words, got $COUNT\" | |
| exit 1 | |
| fi", | |
| ]; |
We should test it there, so that we can reuse the existing test cases that ensure that the same shell scripts work across all executors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, I added a small subset of commands to run with the exec-harness. For now, script-like multi-commands are kinda out of scope, we'll see if we get user demand.
Kept the integration tests in the exec-harness, we'll see if we need more integration tests with pipes and whatnot form the codspeed runner
6e42e0f to
7b1c4ca
Compare
Currently, the codspeed rust core library automatically sends the integration metadata message to the runner. This will be changed later, but in the meantime, remove the second call.
7b1c4ca to
7a24f1d
Compare
not-matthias
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
No description provided.