Move logic in run.sh to rust #98
Conversation
There was a problem hiding this comment.
I intentionally tried to keep the rust code as close to the shell script as possible for now to make the review simpler. There are several refactoring opportunities which I intend to address in a future PR. For now, it should be possible to put the script and rust code side-by-side and confirm that more or less the same thing is happening.
| @@ -24,7 +25,7 @@ for test_dir in tests/*; do | |||
| rm -rf "${test_dir_path}/target" | |||
| rm -f "${test_dir_path}/Cargo.lock" | |||
|
|
|||
| bin/run.sh "${test_dir_name}" "${test_dir_path}" "${test_dir_path}" > /dev/null 2>&1 | |||
There was a problem hiding this comment.
I removed this initially for debugging purposes, but I didn't put it back because the run script isn't supposed to write to stdout / stderr anyway. Suppressing it only leads to more confusion when something does go wrong.
| @@ -1,9 +1,12 @@ | |||
| [package] | |||
| name = "transform-output" | |||
| name = "rust_test_runner" | |||
There was a problem hiding this comment.
The rename reflects that all test running logic is now in rust, not just the transformation of cargo's output.
| version = "0.1.0" | ||
| authors = ["Peter Goodspeed-Niklaus <[email protected]>"] | ||
| edition = "2018" | ||
| edition = "2021" |
There was a problem hiding this comment.
There are always compiler warnings when some code is doing something that was deprecated / will change in a future edition. Since we didn't get any warnings for 2018, it is safe to upgrade.
| @@ -1,4 +1,5 @@ | |||
| { | |||
| "version": 2, | |||
There was a problem hiding this comment.
I believe I actually caught a bug here? For two test cases, the run.sh script would not emit the version property.
| std::env::set_current_dir(&cli_args.input_dir) | ||
| .context("failed to 'cd' into the input directory")?; | ||
|
|
||
| let mut results_out = String::new(); |
There was a problem hiding this comment.
One thing I did change compared to the logic in run.sh is that the file results.out is not written to anymore. As far as I know, it is not part of the test runner interface. Internally, it is only used to store temporary data. So I left it as a mutable variable instead of writing the content to the filesystem.
There was a problem hiding this comment.
That's perfectly fine as we do indeed not use that file.
| let out = convert( | ||
| serde_json::Deserializer::from_slice(&cargo_output.stdout).into_iter::<TestEvent>(), | ||
| ); | ||
| let mut results_json = serde_json::to_string_pretty(&out)?; |
There was a problem hiding this comment.
This is basically what main.rs contained before. (excluding reading from stdin and writing to stdout) Everything above and below is adapted from the shell script.
| std::env::set_current_dir(&cli_args.input_dir) | ||
| .context("failed to 'cd' into the input directory")?; | ||
|
|
||
| let mut results_out = String::new(); |
There was a problem hiding this comment.
That's perfectly fine as we do indeed not use that file.
Moving all logic to Rust makes it easier to understand what the test runner does. It also paves the way for some more substantial refactoring that may cross the previous boundary between shell script and rust executable.