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

Support multiple Kotest spec instances #860

Merged
merged 3 commits into from
Oct 13, 2023

Conversation

sksamuel
Copy link
Contributor

@sksamuel sksamuel commented Oct 1, 2023

Hi Micronaut folks!

I think the application context should be closed when a Kotest spec finishes executing, otherwise the resources are not cleaned up.

Kotest5Context needs to support multiple instances of specs.

See bug report here: kotest/kotest#3711

@CLAassistant
Copy link

CLAassistant commented Oct 1, 2023

CLA assistant check
All committers have signed the CLA.

@@ -51,6 +51,7 @@ object MicronautKotest5Extension: TestListener, ConstructorExtension, TestCaseEx

override suspend fun afterSpec(spec: Spec) {
contexts[spec.javaClass.name]?.afterSpecClass(spec)
contexts[spec.javaClass.name]?.close()
Copy link
Contributor

@Spikhalskiy Spikhalskiy Oct 2, 2023

Choose a reason for hiding this comment

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

The line above already closes the application context here:

https://github.com/micronaut-projects/micronaut-test/blob/master/test-core/src/main/java/io/micronaut/test/extensions/AbstractMicronautExtension.java#L455

So this addition doesn't really do anything at the moment.

@@ -116,4 +116,7 @@ class MicronautKotest5Context(
)
}

fun close() {
applicationContext.close()
Copy link
Contributor

@Spikhalskiy Spikhalskiy Oct 2, 2023

Choose a reason for hiding this comment

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

This throws NPE now, the applicationContext is stopped and nullified by afterSpecClass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this isn't the fix, but I know what is now.
I'll update it and add a test.

@sksamuel sksamuel marked this pull request as draft October 2, 2023 02:48
@sksamuel sksamuel marked this pull request as ready for review October 2, 2023 03:18
@sksamuel sksamuel changed the title Add close call Support multiple Kotest spec instances Oct 2, 2023
@sksamuel
Copy link
Contributor Author

sksamuel commented Oct 2, 2023

I think this should do it @Spikhalskiy

@Spikhalskiy
Copy link
Contributor

Spikhalskiy commented Oct 2, 2023

@sksamuel I confirm that with this change the issue I reported in kotest/kotest#3711 is resolved, all initialized contexts in the sample project are getting shutdown.
I applied it to our larger project and there are no more thread leaks in the large test set.

@Spikhalskiy
Copy link
Contributor

@dstepanov Anything we can do to advance this PR?

@dstepanov
Copy link
Contributor

Looks fine to me

@sdelamo sdelamo merged commit 68cf708 into micronaut-projects:master Oct 13, 2023
3 checks passed
@sksamuel sksamuel deleted the sks/close branch November 12, 2023 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants