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.

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

Clear outer instances when init test state is called #26644

Merged
merged 1 commit into from Jul 12, 2022

Conversation

Sgitario
Copy link
Contributor

The init test state is called several times depending whether JUnit 5 is configured with the lifecycle method based or class based.

Clearing out the outer instances every time the main class is initialized, ensure that the outer instances are always up to date.

The init test state is called several times depending whether JUnit 5 is configured with the lifecycle method based or class based. 

Clearing out the outer instances every time the main class is initialized, ensure that the outer instances are always up to date.
@Sgitario
Copy link
Contributor Author

These changes are related to this comment: #26556 (comment)

@famod @ivansenic, I added a test to ensure that the number of outer instances is always correct (before it was wrong as you spotted). However, I could not reproduce the issue you were seeing, so can you double-check whether these changes do fix your issues?

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 11, 2022

Failing Jobs - Building 28fc5cf

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 18 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 18 #

- Failing: extensions/smallrye-reactive-messaging-kafka/deployment 
! Skipped: integration-tests/kafka-oauth-keycloak integration-tests/kafka-sasl-elytron integration-tests/reactive-messaging-hibernate-reactive and 2 more

📦 extensions/smallrye-reactive-messaging-kafka/deployment

io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingWorkingAppPropsTestCase.testContinuousTestingScenario3 line 50 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Failed to wait for test run 3 State{lastRun=2, running=true, inProgress=true, run=1, passed=0, failed=1, skipped=0, isBrokenOnly=false, isTestOutput=false, isInstrumentationBasedReload=false, isLiveReload=true}
	at io.quarkus.test.ContinuousTestingTestUtils.waitForNextCompletion(ContinuousTestingTestUtils.java:44)
	at io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingWorkingAppPropsTestCase.testContinuousTestingScenario3(KafkaDevServicesContinuousTestingWorkingAppPropsTestCase.java:50)

@ivansenic
Copy link
Contributor

These changes are related to this comment: #26556 (comment)

@famod @ivansenic, I added a test to ensure that the number of outer instances is always correct (before it was wrong as you spotted). However, I could not reproduce the issue you were seeing, so can you double-check whether these changes do fix your issues?

I could.. Just tell me what's the easiest way to build everything as snapshot? Or what's the best way to test this?

@Sgitario
Copy link
Contributor Author

These changes are related to this comment: #26556 (comment)
@famod @ivansenic, I added a test to ensure that the number of outer instances is always correct (before it was wrong as you spotted). However, I could not reproduce the issue you were seeing, so can you double-check whether these changes do fix your issues?

I could.. Just tell me what's the easiest way to build everything as snapshot? Or what's the best way to test this?

Simply, build mvn clean install -Dquickly and use the version 999-SNAPSHOT. More information in https://github.com/quarkusio/quarkus/blob/main/CONTRIBUTING.md#updating-the-version.

@famod
Copy link
Member

famod commented Jul 11, 2022

I'm sure this will fix the imminent issue because I already tested it with that very line added, but I'll re-test later today.

I'd like to point out though that this will still lead to accumulating outer instances during execution of methods of a default method lifecycle nested class.
I haven't checked yet whether this can cause actual issues or whether it's just inconsistent.
This problem can be observed by adding a printout of the outer instances, e.g. in afterEach.

@ivansenic
Copy link
Contributor

These changes are related to this comment: #26556 (comment)
@famod @ivansenic, I added a test to ensure that the number of outer instances is always correct (before it was wrong as you spotted). However, I could not reproduce the issue you were seeing, so can you double-check whether these changes do fix your issues?

I could.. Just tell me what's the easiest way to build everything as snapshot? Or what's the best way to test this?

Simply, build mvn clean install -Dquickly and use the version 999-SNAPSHOT. More information in https://github.com/quarkusio/quarkus/blob/main/CONTRIBUTING.md#updating-the-version.

Will do, hopefully today..

@ivansenic
Copy link
Contributor

@Sgitario Took me some time, but here it is:

✔️ existing tests are now working fine
✔️ using @AfterAll with the @TestInstance(TestInstance.Lifecycle.PER_CLASS) works as well

Thanks a lot 👍

@Sgitario
Copy link
Contributor Author

@Sgitario Took me some time, but here it is:

heavy_check_mark existing tests are now working fine heavy_check_mark using @AfterAll with the @TestInstance(TestInstance.Lifecycle.PER_CLASS) works as well

Thanks a lot +1

Thanks for confirming it!

Copy link
Member

@famod famod left a comment

Choose a reason for hiding this comment

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

Works as expected. Sorry for the delay!

Let's merge this now and improve later the instance accumulation issue that I mentioned earlier.

@famod famod merged commit 4e046f5 into quarkusio:main Jul 12, 2022
@quarkus-bot quarkus-bot bot added this to the 2.11 - main milestone Jul 12, 2022
@Sgitario Sgitario deleted the fix_tests branch July 13, 2022 04:10
@gsmet gsmet modified the milestones: 2.11.0.CR1, 2.10.3.Final Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants