action #93342
open
Exception in log parser should Incomplete if the parser can't parse or throws an exception
Added by livdywan almost 3 years ago.
Updated almost 3 years ago.
Category:
Regressions/Crashes
Description
User story¶
As a test reviewer I need to know that failures are test failures and not due to internal parsing errors.
Acceptance criteria¶
- AC1: Failures in JSON results parsing don't lead to failed tests
- AC2: Tests incomplete if parsing is broken
Suggestions¶
- Check if JSON contains all expected fields e.g. validate against a schema and Incomplete
- Consider wrapping the parsing into an exception handler (try or eval) and Incomplete
Files
Related issues
1 (1 open — 0 closed)
- Copied from action #93324: Exception in log parser leads to failed job with hard to guess root cause added
The incomplete should also have a specific reason. We should then also ignore the incompletes within our monitoring because if a 3rdparty testsuite produces broken output (or uses an unexpected format) that's not really an issue on our side.
Consider wrapping the parsing into an exception handler (try or eval) and Incomplete
Note that the parsing via the test API function parse_extra_log is already been wrapped into an exception handler. Maybe it isn't handled in the right way, though. Note that as far as I see none of the backtraces within the attached log are actually backtraces of exceptions (I see only warnings) so messing with the exception handling likely wouldn't make any difference here. As mentioned in #93324#note-7 I would rather fix the code so warnings cannot occur anymore.
This is how it looks like without any modifications if an exception occurs within the parser code:
Here the provided JSON file is broken and hence the JSON parser throws an exception. As stated in the previous comment, the log attached within the ticket description is a different case because there were only warnings. (Anton takes care of the warnings in https://github.com/os-autoinst/openQA/pull/3926.)
I'm wondering whether this way of handling exceptions within the parser code is actually bad. It is in fact in-line with other failing test API functions. Assuming a bug within the parser by default and not a problem with the data to be parsed seems wrong. But if there's just a problem with the data to be parsed that likely means tests need to be fixed or the parser needs to be adapted. That's similar to fixing tests or creating a new needle after a needle mismatch. In this case we also don't assume by default that os-autoinst's needle matching is broken.
Note that making this an incomplete (with a special reason) would likely also mean to stop the test execution there immediately instead of possibly continuing with the next test module.
So I'm wondering whether we should change this at all - especially since the ticket was mainly motivated by looking at test results where not even an exception has been thrown.
- Status changed from Feedback to Workable
- Assignee deleted (
mkittler)
The ticket doesn't make much sense how I understand it. It would make sense to add a sanity check for the results the parser produced and produce a failure like in the screenshot above if the check fails.
The ticket doesn't make much sense how I understand it. It would make sense to add a sanity check for the results the parser produced and produce a failure like in the screenshot above if the check fails.
So including the other case you mentioned we have these scenarios:
- JSON is invalid, currently raises an exception
- JSON is missing expected fields, currently produces warnings
We don't seem to validate anything at the moment so there might even be more side effects. I think most importantly the behavior should be more well-defined. A modified JSON file shouldn't confuse the code. It should stop trying to process unknown results.
Note that making this an incomplete (with a special reason) would likely also mean to stop the test execution there immediately instead of possibly continuing with the next test module.
True. I'm a bit on the fence here. Stopping the test early seems like a good thing because something here is broken at that point. Maybe it should still fail, though?
- Target version changed from Ready to future
Also available in: Atom
PDF