Catch known exceptions (broken pipe) #1042
Conversation
|
WDYT @mjurc ? |
| /** | ||
| * List of known exception messages. If you want to mask any exception, add its message here | ||
| */ | ||
| private static final List<String> KNOWN_EXCEPTION_MESSAGES = List.of("Broken pipe"); |
There was a problem hiding this comment.
The Broken pipe will be included in the issues we are experiencing now on Podman, but if we ever experience it in a different context then we do right now, it will be logged and skipped. I won't pretend I really do read logs of green jobs.
This, very unlikely, could hide problems. I'm not sure about:
- plain string which doesn't allow for complex pattern like parts of stack trace
- if we go by log lines, you will probably have a hard time to determine what is in the stack trace as it is not one line?
- if we only experience this on Podman, you could only make it known problem on Podman. Our FW already knows whether it is Podman or Docker as we check it on every single run where containers are used.
Maybe you could make it a predicate consuming throwable?
There was a problem hiding this comment.
@michalvavrik I wouldn't mind your opinion about architecture of this either btw.
| /** | ||
| * List of known exception messages. If you want to mask any exception, add its message here | ||
| */ | ||
| private static final List<String> KNOWN_EXCEPTION_MESSAGES = List.of("Broken pipe"); |
There was a problem hiding this comment.
I like the approach of making this a Junit extension, but I really think this bit should be configurable from testsuite or command line. Can it be configured from properties?
There was a problem hiding this comment.
+1 though it's hard to combine with my suggestion, you can treat it as one of, I'll leave it on you
one step ahead of you, I commented first :-D |
|
The think is - do we rather want to have it configured via CLI (something like If we really want to have it configured (or at least enabled) via CLI, then I propose to reduce this to one-sides feature, for broken pipe exclusively, and have it enabled via something like |
|
I agree with your assessment, although I think it's pretty easy to change it to a predicate as you basically c&p what you have into a predicate. I'd prefer to have this enabled on demand with system property. Unless we run into an issue that is omnipresent in the future. I'd suggest |
|
I've changed the checker to use predicates, as @michalvavrik suggested. Now it requires a system property to be effective and you can choose which predicates to use for exception ignoring. To ignore broken pipe, start maven job with |
mocenas
changed the title
|
@michalvavrik waiting for your review. If you like this PR, feel free to merge it (and squash it). |
Summary
Add mechanism to catch known exception and make affected tests skipped, not failed. Motivation for this is to not have failed tests, because of known issues, which cannot be easily disabled - such as quarkusio/quarkus#38334.
To catch this single issue, it should be sufficient to catch exceptions just in
QuarkusScenarioBootstrap(I'm not entire sure if it would be enough). But extending it also to test annotation makes it cover this exception also in tests.Same mechanism can be used to catch any other known exception in the future.
Please check the relevant options
run testsphrase in comment)Checklist: