Migrate to JUnit6

[daniel-raffler]

Hello,

this PR is a draft for migrating our tests from Junit4 to Junit6. Most of the changes are straight forward substitutions, and I tried to avoid any more substantial changes to our test classes for now : if needed, this can still be handled later in a separate PR. To preserve compatibility with other projects that may use out test classes as a basis, I also avoided changes to SolverBasedTest0 and instead cloned the class. The new version SolverBasedTest (without the 0 at the end) uses JUnit6 and is for internal use only, while the older version SolverBasedTest0 (with the 0) is still available for other projects

Current test results with Junit6 can be found here, and here are the same results with JUnit4 tests for comparison

Some caveats to keep in mind:

• There are a few tests missing from the Junit6 results. Most of these are PackageSanity test, which don't appear to be working with JUnit6 and are not picked up by the test runner. However, that still leaves 2 tests unaccounted for
• Skipped tests are not shown in the JUnit6 results. The reason is that JUnit6 sees theses tests as aborted and not skipped as they were (partially) run before an assumption failed. If this is an issue, and we want to keep skipped tests visible, there should probably be a workaround for this
• Most importantly: Parametrized tests are written as testGetArrays5d()[5], instead of testGetArrays5d[Z3], that is, with a parameter index, rather than a parameter value in the [] brackets at the end. The number is "deterministic", so it's possible to figure out which solver is meant by 5, and when running the tests in Intellij (and presumably other IDEs) this also isn't a problem and the parameters are properly printed. However, for the HTML report all we get are these numbers, and I wasn't able to find any way to change that
• We won't be able to get rid of the Junit4 dependency entirely as Truth still depends on it (However, so far this doesn't appear to cause any issues)
• We'll have to update ant on our test systems if we want to use JUnit6. The current version is too old and doesn't support the forkmode attribute that is needed for out tests. However, it should be enough to just update ant, and not the entire system

[daniel-raffler]

Can be reverted once we have ant-junitlauncher from org.apache.ant in Ivy

[PhilippWendler]

• There are a few tests missing from the Junit6 results. Most of these are PackageSanity test, which don't appear to be working with JUnit6 and are not picked up by the test runner. However, that still leaves 2 tests unaccounted for

For the projects where I am maintainer of I would not want to give up PackageSanityTest in favor of Junit 5+.

• Skipped tests are not shown in the JUnit6 results. The reason is that JUnit6 sees theses tests as aborted and not skipped as they were (partially) run before an assumption failed. If this is an issue, and we want to keep skipped tests visible, there should probably be a workaround for this

I think having skipped tests visible in the report is valuable. Otherwise one has no chance of detecting unexpectedly skipped tests.

• Most importantly: Parametrized tests are written as testGetArrays5d()[5], instead of testGetArrays5d[Z3], that is, with a parameter index, rather than a parameter value in the [] brackets at the end. The number is "deterministic", so it's possible to figure out which solver is meant by 5, and when running the tests in Intellij (and presumably other IDEs) this also isn't a problem and the parameters are properly printed. However, for the HTML report all we get are these numbers, and I wasn't able to find any way to change that

This seems also pretty inconvenient.

So we have several disadvantages and to me JUnit 5+ does not seem worth it. Or are there any advantages that would outweigh this?

[daniel-raffler]

Hello Philipp

• There are a few tests missing from the Junit6 results. Most of these are PackageSanity test, which don't appear to be working with JUnit6 and are not picked up by the test runner. However, that still leaves 2 tests unaccounted for

For the projects where I am maintainer of I would not want to give up PackageSanityTest in favor of Junit 5+.

I've now included the vintage test engine for JUnit4 tests, so the PackageSanityTests should be working again

• Skipped tests are not shown in the JUnit6 results. The reason is that JUnit6 sees theses tests as aborted and not skipped as they were (partially) run before an assumption failed. If this is an issue, and we want to keep skipped tests visible, there should probably be a workaround for this

I think having skipped tests visible in the report is valuable. Otherwise one has no chance of detecting unexpectedly skipped tests.

• Most importantly: Parametrized tests are written as testGetArrays5d()[5], instead of testGetArrays5d[Z3], that is, with a parameter index, rather than a parameter value in the [] brackets at the end. The number is "deterministic", so it's possible to figure out which solver is meant by 5, and when running the tests in Intellij (and presumably other IDEs) this also isn't a problem and the parameters are properly printed. However, for the HTML report all we get are these numbers, and I wasn't able to find any way to change that

This seems also pretty inconvenient.

These are really both issues with the legacy/Junit4 xml format that we use to generate test reports. I've now switched the branch to the new opentest format, which should support all Junit5 features and prints all the information that was missing. The CLI tool to generate the reports is still a little rough around the edges, but seems to work fine so far:

You can have a look at a full generated report here

So we have several disadvantages and to me JUnit 5+ does not seem worth it. Or are there any advantages that would outweigh this?

I think Junit5 is more of an evolutionary design with a more modular architecture that is easier to extend. Apart from maybe @Nested tests I don't think there are any new features that we absolutely need. However, since Junit4 is no longer actively developed, and the last release is already 5 years old, we'll have to make the transition at some point. Now that the last major roadblock has been removed seemed like a good time to me

[PhilippWendler]

I've now included the vintage test engine for JUnit4 tests, so the PackageSanityTests should be working again

So we can run JUnit 4 tests with JUnit 5+?

Are there actually any advantages in writing a JUnit 5+ test then? From the diff of the PR it looks as the changes are just changes, not improvements that make the tests nicer.

These are really both issues with the legacy/Junit4 xml format that we use to generate test reports. I've now switched the branch to the new opentest format, which should support all Junit5 features and prints all the information that was missing. The CLI tool to generate the reports is still a little rough around the edges, but seems to work fine so far:

You can have a look at a full generated report here

The report looks nice, but is this stable enough already that we want to use it? And can we export the test data to GitLab as we did before?

Apart from maybe @Nested tests I don't think there are any new features that we absolutely need.

What does @Nested bring us that @RunWith(Enclosed.class) doesn't?

[daniel-raffler]

So we can run JUnit 4 tests with JUnit 5+?

Yes, at least for now. The vintage test engine was marked as deprecated with the JUnit6 release and all tests should be migrated to JUnit5+ in the long term

Are there actually any advantages in writing a JUnit 5+ test then? From the diff of the PR it looks as the changes are just changes, not improvements that make the tests nicer.

I tried to keep this branch as straight forward as possible. There are some additional changes in junit6-nested where I tried to use @Nested to simplify solver based tests with multiple parameters. Other features that might be useful are dynamic tests and parametrized methods, and I believe @baierd was thinking about using jqwik for property based testing

The report looks nice, but is this stable enough already that we want to use it? And can we export the test data to GitLab as we did before?

I've tried here and unfortunately GitLab does not seem to recognize the opentest format. However, we could generate additional xml reports in the JUnit4 format for GitLab to get some basic feedback, although the solver names would likely be missing

What does @Nested bring us that @RunWith(Enclosed.class) doesn't?

I think there can only be one runner in JUnit4, so @RunWith can't be combined with parametrized tests. The nested classes also have to be static, so this can't be used to "curry" parameters for test classes as can be done with @Nested

[PhilippWendler]

So we can run JUnit 4 tests with JUnit 5+?

Yes, at least for now. The vintage test engine was marked as deprecated with the JUnit6 release and all tests should be migrated to JUnit5+ in the long term

For PackageSanityTest we depend on Guava, and I have not heard of any plans for them to move to JUnit 5+. Btw., the same problem affects the various collection test suites that we have in SoSy-Lab Java Common. JUnit 5 support for them was declared as "not planned" in 2018 (google/guava#3039).

Are there actually any advantages in writing a JUnit 5+ test then? From the diff of the PR it looks as the changes are just changes, not improvements that make the tests nicer.

I tried to keep this branch as straight forward as possible. There are some additional changes in junit6-nested where I tried to use @Nested to simplify solver based tests with multiple parameters.

Well, as I mentioned already, using @Nested in order to stack nested classes to overcome JUnit's limitations on parameterized tests to me seems like a giant hack that I certainly would not want to use. Consider what you would need to do in order to just add a single parameter to a test classes and how confusing this would look to a new developer. A factory method for the test parameters similar to what we have now clearly seems like the better solution. So I do not count this use case as an argument for @Nested.

The report looks nice, but is this stable enough already that we want to use it? And can we export the test data to GitLab as we did before?

I've tried here and unfortunately GitLab does not seem to recognize the opentest format. However, we could generate additional xml reports in the JUnit4 format for GitLab to get some basic feedback, although the solver names would likely be missing

Parameterized tests without descriptive names of the parameter values do not seem like a good idea.

What does @Nested bring us that @RunWith(Enclosed.class) doesn't?

I think there can only be one runner in JUnit4, so @RunWith can't be combined with parametrized tests. The nested classes also have to be static, so this can't be used to "curry" parameters for test classes as can be done with @Nested

Cf. above. I don't really see a nice for using non-static nested classes for tests, seems more confusing than beneficial. If you really want something like that, simply make the static inner class inherit from the outer class, then standard Java semantics that everyone know provide basically what @Nested provides AFAIS.