In CI print logs only when any test in a class fails#10620
In CI print logs only when any test in a class fails#10620nineinchnick wants to merge 1 commit intotrinodb:masterfrom
Conversation
| if (memoryHandler == null) { | ||
| return; | ||
| } | ||
| if (context.getFailedTests().size() != 0) { |
There was a problem hiding this comment.
Is there an equivalent to this work for JUnit 5? Otherwise, we’ll lose this functionality when we migrate these tests to it.
There was a problem hiding this comment.
I'll check. I was not aware if we prefer JUnit 5 or TestNG.
There was a problem hiding this comment.
There's a TestWatcher. Is there any existing JUnit 5 test that has a Query Runner or logs something?
There was a problem hiding this comment.
AbstractTestQueryFramework fundamentally requires TestNG due to the annotations in the base class, so this would have to be updated when we migrate it.
There was a problem hiding this comment.
Indeed, but I wouldn’t want to add dependencies that will make it harder or introduce a roadblock to migrating.
There was a problem hiding this comment.
Well, maybe I can start the migration now? Would this be a good enough reason?
| oldLevels.put(handler, handler.getLevel()); | ||
| handler.setLevel(Level.OFF); |
There was a problem hiding this comment.
Logging is global, and tests execute in parallel.
- Concurrently to a test extending from ATQF, there might be some other test that doesn't, and we switch its logs off
- when two instances of ATQF initialize concurrently, the second one may record
oldLevelsasOFFs, and dutifully restore them. This would disable logging for ever, for all tests
There was a problem hiding this comment.
What controls this? I naively thought parallelism is limited to classes: https://github.com/trinodb/trino/blob/master/pom.xml#L79
There was a problem hiding this comment.
methods probably means that any two methods can run in parallel, so including two methods for same class, for same test instance, and for different classes and instances
classes probably means that methods from a single class are not supposed to run in parallel (not sure how this plays with class hierarchies)
There was a problem hiding this comment.
I found this article that has some examples with multiple classes and looks like you're right. I'll close this PR then.
There was a problem hiding this comment.
if i weren't right at least to some extent, i wouldn't spend time on apache/maven-surefire#407 :)
4 participants
The goal of this change is to produce fewer logs and make it easier to find the root cause of a failure. The suggested solution is to skip logs of test classes where all tests succeeded.
Supersedes #10592. It's better, because:
It works on the class level to allow running tests in parallel on a method level. So huge test classes that produce lots of logs will still have the same issue, but it'll reduce the noise if a failure happens in some other smaller test class in the same module.