step line numbers are recorded when protocol is loaded (#65) #121
Conversation
ghost
assigned 
|
i hadn't heard about draft pull requests yet :) |
This change depends on a fork of yaml-rust, https://github.com/hallettj/yaml-rust/tree/load_from_str_with_markers
hallettj
force-pushed
the
jh/indicate-failing-step
branch
from
004feac
to
ca7aa2b
Compare
|
I have an upstream pull request with yaml-rust here: I have updated the branch in this PR to use my fork of yaml-rust, report line numbers in error messages, and adjust tests as necessary to match the new output. |
| executable: PathBuf::from("foo"), | ||
| arguments: vec![], | ||
| }), | ||
| Marker { line: 1, col: 0 } |
There was a problem hiding this comment.
since the marker is a default value, what do you think about making the function above step_with_default_marker and not specifying the marker explicitly in the tests?
There was a problem hiding this comment.
I was kinda torn on this. The tests make an equality comparison between Step values, and if the marker is not specified correctly in the expected value then the test will fail. We could change the tests to compare specific properties within a Step instead of the whole structure - for example we could compare just the command and arguments. In that case calling Step::new would suffice instead of step_with_marker.
Honestly I'm trying to suppress my instinct to put multiple assertions in a test (to check equality for command and arguments) in favor of the idea of testing just one thing in each test. I suppose I could put the commands and arguments into tuples for comparisons to technically get one assertion. I might make that change.
src/protocol/mod.rs
Outdated
| @@ -514,6 +540,12 @@ mod load { | |||
| Ok(result.into_iter().next().unwrap()) | |||
| } | |||
|
|
|||
| fn step_with_marker(command_matcher: CommandMatcher, marker: Marker) -> Step { | |||
There was a problem hiding this comment.
i assume you had to duplicate the function above because here it is in a different sub mod? i ran into this same issue at one point and wasn't sure the best way to share the function between both. seems like you could either add a larger surrounding mod (and move the function up a level), or import the function from the other one like use super::mod_name::function_name. not sure what the best rusty way to do it is, curious what you and @soenkehahn think though.
There was a problem hiding this comment.
I did not think of importing from a sibling module. Maybe I could move step_with_marker into a test helper module. I try not to worry too much about duplicating code in tests. But there is no reason for duplication if it is easy to avoid.
This change depends on a fork of yaml-rust, https://github.com/hallettj/yaml-rust/tree/load_from_str_with_markers
Adds a failing test that asserts that the line number for each protocol step is recorded when loading a protocol. fixes #65
@soenkehahn