Project

General

Profile

action #93342

Exception in log parser should Incomplete if the parser can't parse or throws an exception

Added by cdywan 6 months ago. Updated 5 months ago.

Status:
Workable
Priority:
Normal
Assignee:
-
Category:
Concrete Bugs
Target version:
Start date:
2021-06-01
Due date:
% Done:

0%

Estimated time:
Difficulty:

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
autoinst-log.txt (470 KB) autoinst-log.txt asmorodskyi, 2021-06-01 07:50
screenshot_20210601_151810.png (329 KB) screenshot_20210601_151810.png mkittler, 2021-06-01 13:19
11433

Related issues

Copied from openQA Project - action #93324: Exception in log parser leads to failed job with hard to guess root causeWorkable2021-06-01

History

#1 Updated by cdywan 6 months ago

  • Copied from action #93324: Exception in log parser leads to failed job with hard to guess root cause added

#2 Updated by mkittler 6 months ago

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.

#3 Updated by mkittler 6 months ago

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.

#4 Updated by mkittler 6 months ago

  • Assignee set to mkittler

#5 Updated by mkittler 6 months ago

11433

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.

#6 Updated by mkittler 6 months ago

  • 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.

#7 Updated by cdywan 6 months ago

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?

#8 Updated by okurz 5 months ago

  • Target version changed from Ready to future

Also available in: Atom PDF