Skip to content

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Apr 7, 2025

Follows up on: #47181.

This should fix the flaky tests we've been seeing on PerClassSpyTest

P.S. If #47181 is backported, then this needs to be as well

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 7, 2025
@geoand geoand mentioned this pull request Apr 7, 2025
@gsmet
Copy link
Member

gsmet commented Apr 7, 2025

And we still avoid the memory leaks we were trying to fix?

@gsmet
Copy link
Member

gsmet commented Apr 7, 2025

Should we drop the mocks from TEST_TO_USED_MOCKS after the test?

@geoand
Copy link
Contributor Author

geoand commented Apr 7, 2025

Now we are trying to avoid flaky tests, we don't have any new reports of memory leaks

@gsmet
Copy link
Member

gsmet commented Apr 7, 2025

Yes but my understanding is that we were calling clearInlineMock to avoid memory leaks?

@gsmet
Copy link
Member

gsmet commented Apr 7, 2025

And I'm not very familiar with this so it might be invalid but I'm surprised we keep a reference to all mocks in a static field of MockitoMocksTracker. Shouldn't we clean the CHM once the test has been executed and we have cleaned up the mocks for this test?

This comment has been minimized.

@geoand
Copy link
Contributor Author

geoand commented Apr 7, 2025

Yes but my understanding is that we were calling clearInlineMock to avoid memory leaks?

I really don't remember.

And I'm not very familiar with this so it might be invalid but I'm surprised we keep a reference to all mocks in a static field of MockitoMocksTracker. Shouldn't we clean the CHM once the test has been executed and we have cleaned up the mocks for this test?

Maybe, but that would require more time than I have this week to figure out

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Let's not merge this right away, I will do some archeology.

Also remove mocks from MockitoMocksTracker after the whole test is
executed.
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I pushed an additional commit that seems to work, while still clearing the inline mocks.

Not completely sure that I did nested tests right but I added a test and it seems to work.

@geoand
Copy link
Contributor Author

geoand commented Apr 8, 2025

Thanks a lot for taking a deep look!

Copy link

quarkus-bot bot commented Apr 8, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 7582717.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@geoand geoand merged commit 83929f7 into quarkusio:main Apr 8, 2025
32 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.22 - main milestone Apr 8, 2025
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 8, 2025
@geoand geoand deleted the mock-again branch April 8, 2025 12:30
@gsmet gsmet modified the milestones: 3.22 - main, 3.21.2 Apr 8, 2025
@jmartisk jmartisk modified the milestones: 3.21.2, 3.20.1 Apr 30, 2025
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.

4 participants