Closed alleyshairu closed 1 week ago
Comparing alleyshairu:passthrough_nodejs_and_python_error_message_fix
(81ac320) with main
(8dffbdf)
✅ 38
untouched benchmarks
I agree that for errors exposed by a library a properly defined error enum is very important. However, for this kind of adhoc application level error, I find that we are better off with the flexibility of anyhow.
I think that this error would actually benefit from using anyhow. Currently we get the error:
called `Result::unwrap()` on an `Err` value: Custom { kind: NotFound, error: "Attempted to run the command `npm install` but npm does not exist. Have you installed nodejs?" }
However by swapping to anyhow we can make this a little cleaner.
called `Result::unwrap()` on an `Err` value: Attempted to run the command `npm install` but npm does not exist. Have you installed nodejs?
Personally I wouldnt have written a test here, since the functionality is improving error messages in the test suite, not the application itself. Regardless, I'm quite happy to accept one, it will still bring some value.
I see no issue with comparing strings in the test here, since what we want to test is that the error message is written in a specific way.
In fact the one issue with the test is that we arent properly comparing the strings.
Calling the logic under test twice and comparing the results isnt really testing anything at all.
Instead of calling command_not_found_error_message
in the test, we should provide the specific string
Attempted to run the command `shotover_non_existent_command arg1 arg2` but shotover_non_existent_command does not exist. Have you installed shotover_non_existent_command?
and compare against that.
In general though, your PR is looking good.
Improves error output as defined in the #1829.
Here's a contrived sample output.
I would like you hear your thoughts on the testcase that I have added. The test is currently asserting on a string value instead of some custom error type. I generally do not like matching against string in my tests, but in this instance we are panicking anyways, so it is fine? or we are better off with a custom error type such as
RunCommandError