Introduce configuration parameter for setting default test instance lifecycle semantics #905

sbrannen · 2017-06-29T16:35:55Z

Overview

For teams that prefer per-class test instance lifecycle semantics, in order to prevent such teams from having to copy-n-paste @TestInstance(Lifecycle.PER_CLASS) across the code base, it would be beneficial to introduce a configuration flag (e.g., system property) that allows the default test instance lifecycle semantics to be set on a per-project basis (e.g., for a build).

Switching the default semantics would remain an opt-in decision but would be considerably less intrusive with the configuration flag.

See #419 (comment) for background information.

In addition, we could introduce support for setting ConfigurationParameters via a properties file (e.g., a junit.properties file in the root of the classpath). This would allow such configuration to be contained in the project itself (and checked into a VCS) as opposed to having to set such a flag in the build script as well as in the IDE (perhaps on a per test run basis).

Related Issues

Deliverables

Introduce junit.jupiter.testinstance.lifecycle.default configuration parameter for setting default test instance lifecycle semantics.
- Introduce new constant in org.junit.jupiter.engine.Constants.
- Support enum constant names from TestInstance.Lifecycle, ignoring case.
- Introduce unit tests for the new configuration parameter.
- Introduce integration tests for the new configuration parameter, including override semantics for local config, config params provided to the launcher, and system properties.
Log INFO message if the test instance lifecycle mode has been set via the configuration parameter.
Update JavaDoc for @TestInstance.
Update User Guide.
Update release notes.

The text was updated successfully, but these errors were encountered:

sdeleuze · 2017-06-29T16:50:51Z

Strong +1 for this one and not only from a Kotlin developer perspective, to me @TestInstance(Lifecycle.PER_CLASS) makes sense as the default behavior in Java as well. Since it seems not possible to set that as the default in JUnit 5, being able to set such default globally would be super useful.

sdeleuze · 2017-07-25T14:39:11Z

Since this issue has been postponed to 5.1 backlog (I can understand there is some concern introducing a different mode of execution of the tests based on configuration), I was wondering if we could find a better alternative solution.

For example, can't we turn this issue into enabling automatically PER_CLASS on classes with non-static methods annotated with @BeforeAll and @AfterAll rather than just failing in such case?

sbrannen · 2017-07-25T16:55:53Z

For example, can't we turn this issue into enabling automatically PER_CLASS on classes with non-static methods annotated with @BeforeAll and @AfterAll rather than just failing in such case?

That's certainly an interesting idea.

But I'm concerned that it would come as a surprise to developers who simply forgot to type static but meant to.

@junit-team/junit-lambda What do the rest of you think?

smoyer64 · 2017-07-25T17:09:24Z

it would come as a surprise to developers

I agree - I think feature switches that change the default behavior should be specified explicitly.

sdeleuze · 2017-07-25T17:31:58Z

Ok, I understand.

Is supporting @TestInstance(Lifecycle.PER_CLASS) at package level something we could consider?

marcphilipp · 2017-07-27T12:23:07Z

I still think making it the default for test classes written in Kotlin would be a clear rule and solve your problem...

@junit-team/junit-committers Thoughts?

sdeleuze · 2017-07-27T14:07:54Z

@marcphilipp I guess I missed your proposal, but indeed that would be even better, so big +1 for that !!!

The code for detecting Kotlin classes is straightforward and does not need any dependencies:

private static boolean isKotlinClass(Class<?> clazz) {
	for (Annotation annotation : clazz.getDeclaredAnnotations()) {
		if (annotation.annotationType().getName().equals("kotlin.Metadata")) {
			return true;
		}
	}
	return false;
}

sbrannen · 2017-07-28T10:13:54Z

I still think making it the default for test classes written in Kotlin would be a clear rule and solve your problem...

@junit-team/junit-committers Thoughts?

I think you meant to address the "JUnit Lambda" team. 😛

In any case, I don't have a problem with making it the default for classes written in Kotlin.

Kotlin developers could always explicitly revert back to per-method semantics if desired.

sbrannen · 2017-07-28T10:14:17Z

@sdeleuze, thanks for the isKotlinClass() snippet!

sbrannen · 2017-07-28T11:57:52Z

FYI: I just created #991 to enable per-class lifecycle semantics by default for Kotlin test classes.

JLLeitschuh · 2017-08-01T16:20:19Z

Test classes written in the Kotlin programming language are now executed with @TestInstance(Lifecycle.PER_CLASS) semantics by default.

I read this in the release notes and was kind of surprised by this change. One of the things that I like about Junit over TestNG is that each test it executed in its own instance of the test. I don't want state to be shared between tests. I write a lot of my unit tests in Kotlin and this change would probably break many of my tests. Not only that but if a user were to go through and convert their Java tests to Kotlin they would end up seeing different results which could lead to quite a bit of confusion.

Is there some reason why this should be the default for kotlin? Personally, I don't think this is the right decision.

https://martinfowler.com/bliki/JunitNewInstance.html

I would personally be fine with this being configurable but having this be the default for junit RC2 is enough to prevent me from moving my tests forward to this release.

marcphilipp · 2017-08-01T17:37:08Z

Just so everyone is on the same page, this is what it looks like to use PER_METHOD with @BeforeAll/@AfterAll from Kotlin:

@TestInstance(PER_METHOD)
class SimpleKotlinTestCase {

    companion object {
        @JvmStatic
        @BeforeAll
        fun beforeAll() {
            println("beforeAll")
        }

        @JvmStatic
        @AfterAll
        fun afterAll() {
            println("afterAll")
        }
    }

    @BeforeEach
    fun beforeEach(testInfo: TestInfo) {
        println("beforeEach: " + testInfo)
    }

    @AfterEach
    fun afterEach(testInfo: TestInfo) {
        println("afterEach: " + testInfo)
    }

    @Test
    fun firstTest() {
        println("firstTest")
    }

    @Test
    fun secondTest() {
        println("secondTest")
    }
}

Having to declare the companion object and using @JvmStatic did strike us as ugly and non-obvious, thus the change in #991.

It would be great to get a representative survey of all users who use Jupiter with Kotlin. Otherwise, it's very hard to decide this trade-off. Any ideas?

@sdeleuze Did I state your main concerns with using PER_CLASS correctly or did I miss something?

sdeleuze · 2017-08-01T17:46:18Z

@marcphilipp Yes

If you prefer making it configurable via a global config or a static field, that's would be also fine to me and would be appreciated by a lot of users including non Kotlin ones IMO. What would be a pain would be to specify @TestInstance(PER_CLASS) on each tests.

marcphilipp · 2017-08-01T17:47:05Z

On the other hand, it's only 4 additional lines, and using @BeforeAll/@AfterAll is not something you need in every test. So, we could solve it with additional documentation on how to use @BeforeAll/@AfterAll with Kotlin in the User Guide. 🤔

marcphilipp · 2017-08-01T17:48:17Z

If you prefer making it configurable via a global config or a static field, that's would be also fine to me and would be appreciated by a lot of users including non Kotlin ones IMO. What would be a pain would be to specify @TestInstance(PER_CLASS) on each tests.

How would the static field approach work?

JLLeitschuh · 2017-08-01T17:49:01Z

I 100% expect that this is what I would need to do in order to correctly use @BeforeAll and @AfterAll in kotlin given what I know of the language.

The problem with kotlin is that if you don't do this then you end up with lots of code like this:

class `Some class with a nice name` {
    lateinit val someList : MutableList<Int>
    @BeforeEach
    fun beforeEach() {
        someList = mutableListOf(30)
    }
    @Test
    fun `test that modifies someList`() {
        // Change some list somehow
    }
}

VS

The far simpler:

class `Some class with a nice name` {
    val someList = mutableListOf(30)
    @Test
    fun `test that modifies someList`() {
        // Change some list somehow
    }
}

Having to put lateinit everywhere becomes quite a bit of an annoyance after a while.

JLLeitschuh · 2017-08-01T17:52:01Z

You could do something like mockito where they have a config switch that they put in a resource file:
https://github.com/mockito/mockito/wiki/What%27s-new-in-Mockito-2#unmockable
If you want to enable the mocking of final classes then they expect you to put:

mock-maker-inline

in a file in: src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker

marcphilipp · 2017-08-01T18:03:03Z

If we decide to go that route, we could introduce a general mechanism for reading default ConfigurationParameters from a Properties file, e.g. junit-platform-config.properties (on the classpath).

This file could look like this:

junit.extensions.autodetection.enabled = true
junit.testinstance.lifecycle.default = PER_CLASS

We could read the file when building LauncherConfigurationParameters. Any property passed explicitly to LauncherDiscoveryRequestBuilder (e.g. from an IDE) would override those from the file.

@junit-team/junit-lambda Thoughts?

mlevison · 2017-08-01T18:05:29Z

Well I'm not @Team - however I will take the chance to chime in. As I said in #991: I myself would prefer a code based solution - i.e. Declare a static variable TEST_LIFECYCLE; If doesn't exist the default JUnit semantics exist; if does exist the declared semantics are used. I would make the semantics per test runner launch. I'm always troubled by config files because people often forget to look there. At least the code based approach has slight better visibility. Cheers Mark

…

On Tue, Aug 1, 2017 at 2:03 PM, Marc Philipp ***@***.***> wrote: If we decide to go that route we could introduce a general mechanism for reading default ConfigurationParameters from a Properties file, e.g. junit-platform-config.properties. This file could look like this: junit.extensions.autodetection.enabled = true junit.testinstance.lifecycle.default = PER_CLASS We could read the file when building LauncherConfigurationParameters. Any property passed explicitly to LauncherDiscoveryRequestBuilder (e.g. from an IDE) would override those from the file. @junit-team/junit-lambda <https://github.com/orgs/junit-team/teams/junit-lambda> Thoughts? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#905 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAImZsXPDHsb50kD5Aj1wUFXMyrqhhN1ks5sT2hYgaJpZM4OJj_E> .

kcooney · 2017-08-05T02:45:56Z

@sbrannen thanks for the explanation

I'm not to trilled with the idea of specifying the lifecycle semantics via a system property. That would likely lead to the possibility the setting being different when tests are run via a build tool vs when debugging in an IDE. Using a config file stored as a Java resource would be preferable. Regardless, a warning in the docs might be a good idea.

kcooney · 2017-08-05T02:56:20Z

@JLLeitschuh FYI: I created #1006 to track my proposal in #905 (comment)

See junit-team/junit5#905 I will configure PER_CLASS by default when available

WIP Issue: #905

sbrannen · 2017-08-11T21:10:30Z

FYI: for anyone interested in the progress on this issue...

Here's the code: a916e47

All that's left is documentation.

sdeleuze · 2017-08-12T05:16:22Z

In a Gradle project like https://github.com/sdeleuze/spring-kotlin-functional, what would be the recommended way to change the default to PER_CLASS with this improvement?

Is there a way to set the configuration parameter at Gradle level? Or should I set a system property?

sbrannen · 2017-08-12T19:35:40Z

Hi @sdeleuze,

Unfortunately there is currently no native support for setting a configuration parameter via the console launcher, Maven Surefire plugin, or Gradle plugin.

Thus, the only option from the build perspective is to set a system property.

For the JUnit Platform Gradle Plugin, you can achieve that as discussed here: #475 (comment).

sbrannen · 2017-08-12T19:40:53Z

@sdeleuze,

FYI: I raised #1015 to address the lacking support for setting configuration parameters with the Gradle plugin.

WIP Issue: #905

@testinstance

Prior to this commit, the test instance lifecycle mode could only be changed from the default per-method value to per-class by annotating every single test class or test interface with @testinstance(PER_CLASS). This commit addresses this issue by introducing a new configuration parameter that allows the default test instance lifecycle semantics to be set on a per-project basis (e.g., for a build). Specifically, the default test instance lifecycle mode can now be set via a configuration parameter or JVM system property named `junit.jupiter.testinstance.lifecycle.default` with a value equal to one of the enum constants in TestInstance.Lifecycle. Issue: #905

@testinstance

Prior to this commit, the test instance lifecycle mode could only be changed from the default per-method value to per-class by annotating every single test class or test interface with @testinstance(PER_CLASS). This commit addresses this issue by introducing a new configuration parameter that allows the default test instance lifecycle semantics to be set on a per-project basis (e.g., for a build). Specifically, the default test instance lifecycle mode can now be set via a configuration parameter or JVM system property named `junit.jupiter.testinstance.lifecycle.default` with a value equal to one of the enum constants in TestInstance.Lifecycle. Issue: #905

sbrannen · 2017-08-12T20:57:51Z

Resolved in master in commit f750c85.

Issue: #905

JLLeitschuh · 2017-08-14T14:47:51Z

How does that constant work when running tests in IntelliJ? Will IntelliJ pickup that the gradle plugin has that default configured?

sbrannen · 2017-08-14T18:18:33Z

How does that constant work when running tests in IntelliJ?

Unless IntelliJ provides explicit support for setting configuration parameters to pass to the JUnit Platform Launcher (and I don't think it does yet), your only option (currently) is to set the configuration parameter as a JVM system property for the run configuration in IDEA (or whatever it's called in IDEA -- "run configuration" is the term in Eclipse).

Will IntelliJ pickup that the gradle plugin has that default configured?

Nope. There's currently no explicit support for setting configuration parameters in the Gradle plugin (although there is a work-around). That will be addressed in #1015.

However, the best (i.e., the only robust) solution is the use of the JUnit Platform configuration file which I'll be committing soon in conjunction with #1003. 😉

sbrannen · 2017-08-14T20:41:39Z

@sdeleuze,

In Gradle project like https://github.com/sdeleuze/spring-kotlin-functional, what would be the recommanded way to change the default to PER_CLASS with this improvement? Is there a way to set the configuration parameter at Gradle level ? Or should I set a system property ?

Now that I've resolved #1003, you can give the configuration file approach a try against the latest snapshots.

If you do try it out, please let us know how it works for you.

Cheers

sdeleuze · 2017-08-15T08:29:03Z

@sbrannen Works as expected, and seems a good outcome, thanks !!! sdeleuze/spring-kotlin-functional@7aef220

sbrannen · 2017-08-15T21:14:16Z

Works as expected, and seems a good outcome, thanks !!! sdeleuze/spring-kotlin-functional@7aef220

Cool!

Thanks for letting us know.

@testinstance

Prior to this commit, the test instance lifecycle mode could only be changed from the default per-method value to per-class by annotating every single test class or test interface with @testinstance(PER_CLASS). This commit addresses this issue by introducing a new configuration parameter that allows the default test instance lifecycle semantics to be set on a per-project basis (e.g., for a build). Specifically, the default test instance lifecycle mode can now be set via a configuration parameter or JVM system property named `junit.jupiter.testinstance.lifecycle.default` with a value equal to one of the enum constants in TestInstance.Lifecycle. Issue: junit-team#905

Issue: junit-team#905

sbrannen added component: Jupiter status: team discussion theme: execution type: enhancement labels Jun 29, 2017

sbrannen added this to the 5.0 M6 milestone Jun 29, 2017

sbrannen mentioned this issue Jun 29, 2017

Allow @BeforeAll and @AfterAll methods to be non-static #419

Closed

14 tasks

marcphilipp modified the milestones: 5.0 M6, 5.1 Backlog Jul 18, 2017

sbrannen mentioned this issue Jul 28, 2017

Enable per-class test instance lifecycle mode by default for Kotlin classes #991

Closed

2 tasks

sbrannen changed the title ~~Introduce configuration flag for setting default test instance lifecycle semantics~~ Introduce configuration parameter for setting default test instance lifecycle semantics Aug 4, 2017

sdeleuze added a commit to sdeleuze/spring-kotlin-functional that referenced this issue Aug 5, 2017

Explicit @testinstance(PER_CLASS)

697cef1

See junit-team/junit5#905 I will configure PER_CLASS by default when available

sbrannen added a commit that referenced this issue Aug 7, 2017

Introduce config param for default test instance lifecycle

14ec8d7

WIP Issue: #905

sbrannen added a commit that referenced this issue Aug 10, 2017

Introduce config param for default test instance lifecycle

845ef19

WIP Issue: #905

sbrannen added a commit that referenced this issue Aug 10, 2017

Introduce config param for default test instance lifecycle

d54d4df

WIP Issue: #905

sbrannen added a commit that referenced this issue Aug 11, 2017

Introduce config param for default test instance lifecycle

a916e47

WIP Issue: #905

sbrannen mentioned this issue Aug 12, 2017

Introduce support for configuration parameters in console launcher and build plugins #1015

Closed

6 tasks

sbrannen added a commit that referenced this issue Aug 12, 2017

Introduce config param for default test instance lifecycle

7c62281

WIP Issue: #905

sbrannen added a commit that referenced this issue Aug 12, 2017

Introduce config param for default test instance lifecycle

0718b23

WIP Issue: #905

sbrannen added a commit that referenced this issue Aug 12, 2017

Introduce config param for default test instance lifecycle

34d813d

WIP Issue: #905

sbrannen closed this as completed Aug 12, 2017

ghost removed the status: in progress label Aug 12, 2017

sbrannen added a commit that referenced this issue Aug 12, 2017

Add warning to User Guide re: default test instance lifecycle

589a04f

Issue: #905

sbrannen added the component: Kotlin label Aug 19, 2017

Andrei94 pushed a commit to Andrei94/junit5 that referenced this issue Jun 23, 2018

Add warning to User Guide re: default test instance lifecycle

e16cc3a

Issue: junit-team#905

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce configuration parameter for setting default test instance lifecycle semantics #905

Introduce configuration parameter for setting default test instance lifecycle semantics #905

sbrannen commented Jun 29, 2017 •

edited

sdeleuze commented Jun 29, 2017 •

edited by sbrannen

sdeleuze commented Jul 25, 2017

sbrannen commented Jul 25, 2017 •

edited

smoyer64 commented Jul 25, 2017

sdeleuze commented Jul 25, 2017

marcphilipp commented Jul 27, 2017

sdeleuze commented Jul 27, 2017 •

edited by sbrannen

sbrannen commented Jul 28, 2017

sbrannen commented Jul 28, 2017

sbrannen commented Jul 28, 2017

JLLeitschuh commented Aug 1, 2017 •

edited

marcphilipp commented Aug 1, 2017 •

edited by sbrannen

sdeleuze commented Aug 1, 2017

marcphilipp commented Aug 1, 2017

marcphilipp commented Aug 1, 2017

JLLeitschuh commented Aug 1, 2017 •

edited

JLLeitschuh commented Aug 1, 2017

marcphilipp commented Aug 1, 2017 •

edited

mlevison commented Aug 1, 2017 via email

kcooney commented Aug 5, 2017

kcooney commented Aug 5, 2017

sbrannen commented Aug 11, 2017

sdeleuze commented Aug 12, 2017 •

edited by sbrannen

sbrannen commented Aug 12, 2017

sbrannen commented Aug 12, 2017

sbrannen commented Aug 12, 2017

JLLeitschuh commented Aug 14, 2017

sbrannen commented Aug 14, 2017

sbrannen commented Aug 14, 2017

sdeleuze commented Aug 15, 2017

sbrannen commented Aug 15, 2017

Introduce configuration parameter for setting default test instance lifecycle semantics #905

Introduce configuration parameter for setting default test instance lifecycle semantics #905

Comments

sbrannen commented Jun 29, 2017 • edited

Overview

Related Issues

Deliverables

sdeleuze commented Jun 29, 2017 • edited by sbrannen

sdeleuze commented Jul 25, 2017

sbrannen commented Jul 25, 2017 • edited

smoyer64 commented Jul 25, 2017

sdeleuze commented Jul 25, 2017

marcphilipp commented Jul 27, 2017

sdeleuze commented Jul 27, 2017 • edited by sbrannen

sbrannen commented Jul 28, 2017

sbrannen commented Jul 28, 2017

sbrannen commented Jul 28, 2017

JLLeitschuh commented Aug 1, 2017 • edited

marcphilipp commented Aug 1, 2017 • edited by sbrannen

sdeleuze commented Aug 1, 2017

marcphilipp commented Aug 1, 2017

marcphilipp commented Aug 1, 2017

JLLeitschuh commented Aug 1, 2017 • edited

JLLeitschuh commented Aug 1, 2017

marcphilipp commented Aug 1, 2017 • edited

mlevison commented Aug 1, 2017 via email

kcooney commented Aug 5, 2017

kcooney commented Aug 5, 2017

sbrannen commented Aug 11, 2017

sdeleuze commented Aug 12, 2017 • edited by sbrannen

sbrannen commented Aug 12, 2017

sbrannen commented Aug 12, 2017

sbrannen commented Aug 12, 2017

JLLeitschuh commented Aug 14, 2017

sbrannen commented Aug 14, 2017

sbrannen commented Aug 14, 2017

sdeleuze commented Aug 15, 2017

sbrannen commented Aug 15, 2017

sbrannen commented Jun 29, 2017 •

edited

sdeleuze commented Jun 29, 2017 •

edited by sbrannen

sbrannen commented Jul 25, 2017 •

edited

sdeleuze commented Jul 27, 2017 •

edited by sbrannen

JLLeitschuh commented Aug 1, 2017 •

edited

marcphilipp commented Aug 1, 2017 •

edited by sbrannen

JLLeitschuh commented Aug 1, 2017 •

edited

marcphilipp commented Aug 1, 2017 •

edited

sdeleuze commented Aug 12, 2017 •

edited by sbrannen