Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support Surefire 3.0.0-M5 #10081

Merged
merged 1 commit into from
Jun 26, 2020
Merged

Support Surefire 3.0.0-M5 #10081

merged 1 commit into from
Jun 26, 2020

Conversation

Tibor17
Copy link
Contributor

@Tibor17 Tibor17 commented Jun 17, 2020

No description provided.

@Tibor17 Tibor17 mentioned this pull request Jun 17, 2020
Merged
@geoand
Copy link
Contributor

geoand commented Jun 19, 2020

Thanks for this!

Can you please rebase onto the latest master? I want to see if CI will pass or not.

@Tibor17
Copy link
Contributor Author

Tibor17 commented Jun 19, 2020

@geoand
There are test failures. Pls see the stacktraces.
Fasterxml missing subtypes, Could not stop Keycloak server.

@geoand
Copy link
Contributor

geoand commented Jun 19, 2020

The reason I am asking is because those tests all pass on master. If they fail with this PR, then it would be nice to know why - what changed that cause them to fail so we can update them as part of this PR

@Tibor17
Copy link
Contributor Author

Tibor17 commented Jun 19, 2020

@geoand
I do not have time to analyse this but i see what everybody can see. See them, open it and see the stacktrace.

@geoand
Copy link
Contributor

geoand commented Jun 19, 2020

I understand that, what I'm saying is that it would be nice to know what changed in Surefire that could cause these failures.

@Tibor17
Copy link
Contributor Author

Tibor17 commented Jun 19, 2020 •
edited
Loading

For instance we are able to handle the exceptions thrown in @BeforeAll methods.
This was a required fix by JUnit5 users.
Also other errors are handled from JUnit5. I think now this handler is more objective.

For instance we had a discussion at the mailing list during the release and one JUnit5 user was opointing to a difference with errors thrown in BeforeAll between 2.22.2 and M5. Finally, we agreed that M5 is objective because the same result is observed by IDEA.

Not sure if these errors with marshaller are JDK related but it would be nice to reproduce it locally with identical platform.

@Tibor17
Copy link
Contributor Author

Tibor17 commented Jun 21, 2020

@geoand
Is somebody investigating the errors found in the log?

@geoand
Copy link
Contributor

geoand commented Jun 21, 2020

Not yet, hopefully we'll look into it come Monday

@Tibor17
Copy link
Contributor Author

Tibor17 commented Jun 21, 2020 •
edited
Loading

@geoand
Obviously the provider JUnitPlatformProvider failed in constructor because the instance fields attempt to instantiate the JUnit5 objects and internal code of JUnit5 org.junit.platform.launcher.core.ServiceLoaderTestEngineRegistry.<clinit> failed as a root cause due to JBoss logger could not find some class.

As I said before, we ignored these errors in the old versions, and now this error is visible exactly before any tests start running.

[ERROR] at java.net.URLClassLoader.findClass(URLClassLoader.java:382)
[ERROR] at java.lang.ClassLoader.loadClass(ClassLoader.java:419)
[ERROR] at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:352)
[ERROR] at java.lang.ClassLoader.loadClass(ClassLoader.java:352)
[ERROR] at java.util.logging.LogManager$1.run(LogManager.java:195)
[ERROR] at java.util.logging.LogManager$1.run(LogManager.java:181)
[ERROR] at java.security.AccessController.doPrivileged(Native Method)
[ERROR] at java.util.logging.LogManager.(LogManager.java:181)
[ERROR] at java.util.logging.Logger.demandLogger(Logger.java:448)
[ERROR] at java.util.logging.Logger.getLogger(Logger.java:502)
[ERROR] at org.junit.platform.commons.logging.LoggerFactory$DelegatingLogger.(LoggerFactory.java:80)
[ERROR] at org.junit.platform.commons.logging.LoggerFactory.getLogger(LoggerFactory.java:51)
[ERROR] at org.junit.platform.launcher.core.ServiceLoaderTestEngineRegistry.(ServiceLoaderTestEngineRegistry.java:32)
[ERROR] at org.junit.platform.launcher.core.LauncherFactory.create(LauncherFactory.java:87)
[ERROR] at org.junit.platform.launcher.core.LauncherFactory.create(LauncherFactory.java:67)
[ERROR] at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.(JUnitPlatformProvider.java:90)
[ERROR] at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
[ERROR] at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
[ERROR] at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
[ERROR] at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
[ERROR] at org.apache.maven.surefire.api.util.ReflectionUtils.instantiateOneArg(ReflectionUtils.java:123)
[ERROR] at org.apache.maven.surefire.booter.ForkedBooter.createProviderInCurrentClassloader(ForkedBooter.java:491)
[ERROR] at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:428)
[ERROR] at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:162)
[ERROR] at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:562)
[ERROR] at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:548)

geoand added a commit to geoand/quarkus that referenced this pull request Jun 22, 2020
@geoand
Ensure that TestResourceManager is only closed once
3a6b5c4 
Without this PR the TestResourceManager gets closed 3 times,
which is causing some issues with new surefire versions

Relates to: quarkusio#10081
@geoand geoand mentioned this pull request Jun 22, 2020
Merged
geoand added a commit to geoand/quarkus that referenced this pull request Jun 22, 2020
@geoand
Ensure that TestResourceManager is only closed once
25c7a2a 
Without this PR the TestResourceManager gets closed 3 times,
which is causing some issues with new surefire versions

Relates to: quarkusio#10081
geoand added a commit to geoand/quarkus that referenced this pull request Jun 22, 2020
@geoand
Avoid doing any xstream related work unless absolutely necessary
8394406 
Relates to: quarkusio#10081
@Tibor17
Copy link
Contributor Author

Tibor17 commented Jun 23, 2020

@geoand
You have fixed these uncovered issues in #10146 ?

@geoand
Copy link
Contributor

geoand commented Jun 23, 2020

That was definitely one issue. Can you rebase the PR onto master so we can see if CI passes now or if there are more issues?

@Tibor17
Copy link
Contributor Author

Tibor17 commented Jun 23, 2020

@geoand
done

@geoand
Copy link
Contributor

geoand commented Jun 24, 2020

All tests pass, great!

Could I trouble to rebase onto master again? The reason I am asking is that we made a change to #10146 and I would like to ensure that nothing broke with regards to this.

Thanks

@gsmet
Copy link
Member

gsmet commented Jun 24, 2020

I just pushed a rebase.

@gsmet gsmet added this to the 1.6.0 - master milestone Jun 24, 2020
@geoand
Copy link
Contributor

geoand commented Jun 24, 2020

Thanks

@Tibor17
Copy link
Contributor Author

Tibor17 commented Jun 24, 2020

@geoand
Pls have alok at this test in Quarkus CI / JDK Java 8 - 242 JVM Tests (pull_request)
It looks like Basic Auth failure
[ERROR] Failures:
[ERROR] FormAuthCookiesTestCase.testCredentialCookieRotation:184->doRegularGet:135 Session cookie WAS NOT eligible for renewal and should have remained the same. ==> expected: <DBCucILj++O9vRjU2h7qq5JbrFrzPUgrm1vYOv2YBh+btNqFJu5oJ4gdk+hjan7Y> but was: <DOh/zejFs/YxwI9Drpw5RbIqYWT4vmRE9MXz+NyN7WAPkF6Bph++h+YBW59uiaBE>
[INFO]
[ERROR] Tests run: 71, Failures: 1, Errors: 0, Skipped: 1

@geoand
Copy link
Contributor

geoand commented Jun 24, 2020 via email

@gsmet gsmet modified the milestones: 1.6.0 - master, 1.7.0 Jun 25, 2020
@gsmet
Copy link
Member

gsmet commented Jun 26, 2020

I force pushed a rebase to fix a few conflicts. Let's wait for CI and merge.

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 26, 2020
@geoand
Copy link
Contributor

geoand commented Jun 26, 2020

Assuming everything works 🤪

geoand
geoand approved these changes Jun 26, 2020
View reviewed changes
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All green, nice!

@geoand
Copy link
Contributor

geoand commented Jun 26, 2020

Let me just check one last thing before merging

@geoand geoand merged commit 3aa4346 into quarkusio:master Jun 26, 2020
@geoand
Copy link
Contributor

geoand commented Jun 26, 2020

Thanks a lot for this!

We'll let you know if we run into any issues

@famod
Copy link
Member

famod commented Jun 26, 2020

@geoand I am wondering whether the quickstarts should be adjusted as well?

@geoand
Copy link
Contributor

geoand commented Jun 26, 2020

For 1.7 where this change will take effect, it makes sense

@famod
Copy link
Member

famod commented Jun 26, 2020 •
edited
Loading

It seems there are still some modules picking up Surefire 2.22.0 from jboss-parent, e.g. quarkus-devtools-common.
(e.g. https://github.com/quarkusio/quarkus/pull/10311/checks?check_run_id=811868118#step:11:3822)

I'll have a look.

@geoand
Copy link
Contributor

geoand commented Jun 26, 2020

Thanks @famod!

This was referenced Jun 26, 2020
Merged
Merged
johnaohara pushed a commit to johnaohara/quarkus that referenced this pull request Jun 29, 2020
@geoand @johnaohara
Ensure that TestResourceManager is only closed once
dfd9940 
Without this PR the TestResourceManager gets closed 3 times,
which is causing some issues with new surefire versions

Relates to: quarkusio#10081
johnaohara pushed a commit to johnaohara/quarkus that referenced this pull request Jun 29, 2020
@geoand @johnaohara
Avoid doing any xstream related work unless absolutely necessary
345d5c5 
Relates to: quarkusio#10081
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this pull request Sep 4, 2022
Avoid doing any xstream related work unless absolutely necessary
a78d704 
    Relates to: quarkusio/quarkus#10081
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this pull request Sep 4, 2022
Ensure that TestResourceManager is only closed once
c2364c7 
    Without this PR the TestResourceManager gets closed 3 times,
    which is causing some issues with new surefire versions

    Relates to: quarkusio/quarkus#10081
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoand geoand geoand approved these changes

Assignees
No one assigned
Labels
area/amazon-lambda area/dependencies Pull requests that update a dependency file area/funqy area/kubernetes area/maven triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Milestone
1.7.0.CR1
Development

Successfully merging this pull request may close these issues.
4 participants
@Tibor17 @geoand @gsmet @famod