Repository navigation
tests: use the json visitor to implement a simple path matcher - #207
Conversation
367ddd5 to
86d4c06
Compare
There was a problem hiding this comment.
Lol you and I had a mid-air collision on this. Sorry about that -- I did not realize, and mine has already been merged in: https://github.com/rust-minidump/minidump-writer/blob/main/tests/linux_minidump_writer_soft_error.rs#L35
I don't know which one we should keep, honestly. Yours reads better, and it looks like yours might be a bit more strict about what it will accept. Whether that's good or bad might be debatable -- My version of this allows you to use a function to evaluate whether each terminal node has a specific value and path leading to it. Flexible, but... More work to maintain.
Curious to hear your thoughts?
|
I think having a json walker is probably a good idea in the long run, however I do think we need to have Let me rework this commit to use your helper instead, see how it goes. |
86d4c06 to
0ec6eb4
Compare
|
|
||
| // Ensure the MozSoftErrors stream contains the expected errors | ||
| let expected_errors = vec![ | ||
| ( |
There was a problem hiding this comment.
So... With the understanding that I like the way you actually implemented this, I'm torn on re-adding the strictness that this implies.
The reason I moved to a bit more of a loose matching is experience with this crate -- Basically, every time somebody goes and refactors things a bit so that it slightly changes the structure of the errors, this test breaks.
So, I figured it might be worth stepping back and asking "What are the bits that we really care about, and what is just noise?"
And overall, my conclusion is: If the thing that we've done is "Ask failspot to return EPERM on StopProcess", then the thing we should be checking is "there exists a soft error somewhere that indicates StopProcess failed with EPERM".
So -- You see how the code I have accomplished that minimalism, and so I think that the structure like this is actually over-specified for what we actually need to test.
What do you think?
There was a problem hiding this comment.
To be completely honest, I didn't know what the purpose of the test really was. I'm all for loosening it, however I do think your approach is really hurtful in terms of readability of the test. I want to see what you're expecting out of the test, not how you go about finding that info!
Basically, something like this:
let dump = Minidump::read_path(tmpfile.path()).expect("failed to read minidump");
assert_minidump_soft_errors_match_all(&dump, &[
ErrorPattern::value(EPERM).with_ancestor("StopProcessFailed"),
ErrorPattern::value(AuxvError::InvalidFormat).with_ancestor("FillMissingAuxvInfoErrors"),
ErrorPattern::value(EPERM).with_ancestor("ReadThreadNameFailed"),
ErrorPattern::value(EPERM).with_ancestor("SuspendThreadErrors"),
ErrorPattern::value(EPERM).with_ancestor("WriteCpuInformationFailed"),
]);There was a problem hiding this comment.
Yep -- This is way better!
70e95e6 to
886a0af
Compare
There was a problem hiding this comment.
LGTM! Weird... Is panic not already part of the prelude?
| @@ -1,10 +1,13 @@ | |||
| use core::panic; | |||
There was a problem hiding this comment.
I assume this was needed for some reason?
There was a problem hiding this comment.
No it's not, I think it's a snippet thingy that was too zealous, and a committer that was in a hurry. Fixed.
The adhoc visitor in soft_error_stream_content is checking for some basic structure properties without being as strict as assert_soft_errors_in_minidump is. This patch simply hides the struct walking behind a helper, so that the intent is clearly shown in the test itself, and the nitty-gritty of walking through the JSON are hidden away in a helper.
|
Yes of course, I took the opportunity to change the description of the PR to reflect how it has evolved since (basically pasted the commit description). |
886a0af to
6a1e60d
Compare
The adhoc visitor in soft_error_stream_content is checking for some
basic structure properties without being as strict as assert_soft_errors_in_minidump
is. This patch simply hides the struct walking behind a helper, so that
the intent is clearly shown in the test itself, and the nitty-gritty of
walking through the JSON are hidden away in a helper.
Extracted from #195