-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tests to make sure our test framework tolerates maven reruns even with test profiles #46945
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
a39b828
to
7d99596
Compare
integration-tests/maven/src/test/java/io/quarkus/maven/it/SurefireRerunFailedTestsIT.java
Show resolved
Hide resolved
d183884
to
602b054
Compare
Rebasing in the hopes that the problem with the build reports has been fixed upstream. |
There's something odd here, the build report is failing with the flake message being null, which is the very very first time I actually see that. |
Ohh, that's got to be related to my changes, then. I saw Qute and assumed it wasn't me. Although I don't know why it would be happening, because the failure is just an ordinary test failure. I'll have a look to see if there's some exception message I've failed to fill in somewhere. I guess it also means my test will turn up on every flake report, which is a bit irritating, even if I make sure the message is "deliberate failure" or something informative. I wonder if there's a property I can set to exclude it? |
602b054
to
06e875c
Compare
06e875c
to
39c51b8
Compare
I think the problem might have been caused by failing with thrown |
I deployed a fix to both the Bot and the Build Reporter Action. |
Status for workflow
|
Either your fix or my unwrapping of the exception sorted it out, @gsmet. Do you think we should try and exclude those failures from the build report? Otherwise that's going to be annoyingly noisy. |
Ah, my, yes, this is going to be very annoying. Let's not merge right away, then. I'll mark it as draft for now. |
We probably want some kind of "expected flakes" config on the build reporter. |
Yes. We need to add some infrastructure for that. I'll have a look soon. |
Reproducer for #46048
As expected, this passes on
main
. To my surprise, it also passes on the latest version of #34681. I was assuming I had the test wrong or wasn't testing the correct scenario (after all, never trust a test you haven't seen fail). So I checked against last week's version of #34681, and these tests failed.It appears a miracle has happened, and #46048 has been fixed sometime in the last week, without needing any maven changes. When I looked at this problem last time, the tests were not reloaded by the FacadeClassLoader on the second test run, so they'd run in the old classloader against a closed Quarkus application, and that worked about as well as you'd expect. This time, I've confirmed that on the second run the tests do get freshly loaded, and so everything works.
I can also see in CI that failing LGTM tests are being rerun, and they're failing in the LGTM code each time, not in the test framework code. So that's a useful extra confirmation, si
I'm not 100% sure what fixed it, but I think it's perhaps a combination of @aloubyansky's suggestions about cleaner test file discovery, and my work to close classloaders and leak less.