Replace JUnit 4 by JUnit 5

[paulheinr]

With this PR, JUnit 4 should be replaced by JUnit 5. Testcases that don't use the @Rule annotation can automatically be converted by IntelliJ. We need another logic for the MatsimTestUtils.

It's main purpose is to provide functions to get the input and output directories. This data is collected by the starting() and finished() methods. JUnit 5 provides the same logic via @BeforeEach and @AfterEach.

So my proposition is the following: Instead of having a rule field, classes which use the MatsimTestUtils inherit the functionality from an abstract parent test class. It provides the same functionality.

I implemented this for two example test classes.

Closes #1354.

[kainagel]

didn't we have that earlier and moved away from it? Anybody remembers the reasons? @mrieser ?

One reason may be that we want to wean people away from inheritance. I can see that it will be plausible for tests but not elsewhere, but if people see it a lot, even in tests, they will start using it elsewhere as well.

But these are just arguments, not a decision.

[michalmac]

I think we discussed it several times, including this issue: #1354.

I am leaning more towards hard switching from junit 4 to 5. The problem with the soft switch (using junit-vintage-engine) is that we start supporting two testing standards. Currently our tests are already complex and require knowing some tricks. Having both 4 and 5 will increase the cognitive load (which is against the idea of having tests very simple to understand and maintain).

But the hard switch may be painful, so maybe we could do it gradually for some contribs (as an intermediate solution between the global hard and soft switch).

[michalmac]

I think we discussed it several times, including this issue: #1354.

I am leaning more towards hard switching from junit 4 to 5. The problem with the soft switch (using junit-vintage-engine) is that we start supporting two testing standards. Currently our tests are already complex and require knowing some tricks. Having both 4 and 5 will increase the cognitive load (which is against the idea of having tests very simple to understand and maintain).

But the hard switch may be painful, so maybe we could do it gradually for some contribs (as an intermediate solution between the global hard and soft switch).

Please do not take my comment as a voice against this PR. I just wanted to give a broader context.

[paulheinr]

Thanks for your comments!

Have a look at MatsimJunit5TestExtension.java and the test case RunEvExampleTest.java. I implement the TestUtils as an Extension, which has pretty much the same syntax as Rule. (https://www.baeldung.com/junit-5-registerextension-annotation). I think this could help to get rid of JUnit4 much easier. I agree that we don't want two testing standards.

However, I do see a drawback: Both the rule and the extension hold information that is test case specific. They get their data somehow "magically". With inheritance, it's much more explicit. So the getInputDirectory() function belongs to the test itself and not to the utils.

But because of the easier refactoring, I would vote for the extension.

[michalmac]

Have a look at MatsimJunit5TestExtension.java and the test case RunEvExampleTest.java. I implement the TestUtils as an Extension, which has pretty much the same syntax as Rule. (https://www.baeldung.com/junit-5-registerextension-annotation). I think this could help to get rid of JUnit4 much easier. I agree that we don't want two testing standards.

Thanks. I think I did not read this PR carefully (or somehow missed the last commit). I like the approach with the extension.

If it is as you say that IntelliJ converts everything but rules, then the migration seems quite straightforward.

[paulheinr]

It wasn't that easy as expected, but I made some progress. ToDos are:

• Remove Simple Json dependency (using too old JUnit 4)
• Migrate MatsimTestUtils
• Migrate Assertions
• Migrate Test annotations
• Migrate Parameterized (around 30 classes use parameterization. no good migration tool exists -> needs to be done by hand)

Fortunately, there are some automatic migration tools which are better than the built in ones of IntelliJ: https://docs.openrewrite.org/recipes/java/testing/junit5/

[mrieser]

didn't we have that earlier and moved away from it? Anybody remembers the reasons?

Probably we used inheritance with JUnit 3 several years ago, but replaced it with the @Rule in Unit 4.

Both the rule and the extension hold information that is test case specific. They get their data somehow "magically". With inheritance, it's much more explicit.

Haha, I should use that reasoning to get rid of Dependency Injection, I always preferred the hard-coded dependencies in the constructors, it made it easier to follow the trail of usages. 😉 (I got used to DI in the mean time, but it was a long way...)

I would prefer the extension over inheritance for the tests.

Thanks for cleaning up the code and getting rid of the old stuff!

[paulheinr]

Haha, I should use that reasoning to get rid of Dependency Injection, I always preferred the hard-coded dependencies in the constructors, it made it easier to follow the trail of usages. 😉 (I got used to DI in the mean time, but it was a long way...)

Yes, DI is indeed magic. But normally if I inject class A into B, A has no information about B. And getting rid of constructors is a very strong argument.
With the extensions it's the other way round. But I think I will also get used to the extension approach 😄

[paulheinr]

I think I'm done with the migration.

Some last remarks:

1. jsprit still depends on junit 4. So within the contribs using jsprit, junit 4 is still available (e.g. freight). But I switched every test case to junit 5.
2. @michalmac There is this AutoResetIdCaches class which resets all the matsim ids after each test. I'm wondering why it's there because my expectation is that test cases should work even if the ids are not reset.
   There is similar functionality in JUnit 5 (https://maciejwalkowiak.com/blog/auto-registering-junit-5-extensions/). Nevertheless, I ran into problems. The reset of ids would be done before each test case. Then some preprocessed ids in fixtures are gone. So if everything works without this class, I would just delete it?

Could you also check the pom files and some random test classes?

[michalmac]

There is this AutoResetIdCaches class which resets all the matsim ids after each test. I'm wondering why it's there because my expectation is that test cases should work even if the ids are not reset.

AutoResetIdCaches resets the cache before each test when running them from maven. It's a maven-based approach, so it probably does not work with IDEs, which is not really great.

Why we need it? The ID cache is a global variable. Without resetting it, some state will leak from one test to another. This often leads to non-deterministic results (e.g. when iterating over ID sets or map keys).

There is similar functionality in JUnit 5 (https://maciejwalkowiak.com/blog/auto-registering-junit-5-extensions/).

This looks interesting, because it means that it should work also in IDEs.

Nevertheless, I ran into problems. The reset of ids would be done before each test case. Then some preprocessed ids in fixtures are gone. So if everything works without this class, I would just delete it?

AutoResetIdCaches is also triggered before each test case.

Some time ago I removed lots of static variables/fields from our tests. Maybe I removed (almost) all, but some new have been added in the meantime? Anyway, if there is a fixture that is shared, I suggest we make it non-static and keep on resetting the cache.

[michalmac]

I ran into problems. The reset of ids would be done before each test case. Then some preprocessed ids in fixtures are gone. So if everything works without this class, I would just delete it?

I looked at the build results and started wondering: Do we have builds that show that before removing "auto-reset" we have a failure, and after removing it, the build is green?

[paulheinr]

I looked at the build results and started wondering: Do we have builds that show that before removing "auto-reset" we have a failure, and after removing it, the build is green?

Yes, right now, all builds are green and auto reset is removed. The build before, I switched to the JUnit 5 Extension and got a red build.

Some time ago I removed lots of static variables/fields from our tests. Maybe I removed (almost) all, but some new have been added in the meantime? Anyway, if there is a fixture that is shared, I suggest we make it non-static and keep on resetting the cache.

Even with non-static fixtures there are problems. Take the test class org.matsim.contrib.signals.builder.QSimSignalTest. it has a fixture as class field. When constructing it Ids are created and stored. The tests rely on these Ids. But before each test case, the Ids are reset. I am wondering why it worked with JUnit 4.

[paulheinr]

Hm, it's not green apparently. I tried to find the root cause and it looks like that the assertion, which fails, compares an id which is randomly chosen. When I run the tests of the freightreceiver contrib locally, I everything works fine (even 10times in a row)

[michalmac]

Yes, right now, all builds are green and auto reset is removed. The build before, I switched to the JUnit 5 Extension and got a red build.

The build before is red because it got cancelled: https://github.com/matsim-org/matsim-libs/actions/runs/7197145936.
Could you maybe revert the change and push to see the output.

When I run the tests of the freightreceiver contrib locally, I everything works fine (even 10times in a row)

That's the point. The tests are flaky, sometimes they pass, sometimes not. It all depends whether, for instance, you run one test or many. There are also differences in the order of test execution (e.g. OS, IDE vs mvn).

I think the authors of the tests write them in a way that they work fine when run in the IDE for a given mvn module (e.g. contrib). That is why there is a high chance of not running into problems when running locally.

Even with non-static fixtures there are problems. Take the test class org.matsim.contrib.signals.builder.QSimSignalTest. it has a fixture as class field. When constructing it Ids are created and stored. The tests rely on these Ids. But before each test case, the Ids are reset. I am wondering why it worked with JUnit 4.

The fixture is a non-static field. That means it is created for each test (6 times in this class).

• with auto-resetting - every time the fixture is created, new Ids are created as well (the cache is empty)
• without auto-resetting - every time the fixture is created, ids are obtained from the cache

The problem is with static fields - they are created only once, so there is a risk of transferring some state from one test to another.

[paulheinr]

The build before is red because it got cancelled: https://github.com/matsim-org/matsim-libs/actions/runs/7197145936.
Could you maybe revert the change and push to see the output.

I did now. There are lots of failures. One can reproduce them locally.

The fixture is a non-static field. That means it is created for each test (6 times in this class).

I don't think so. Because it's a class field, it gets created once. The same problems applies for org.matsim.contrib.dvrp.path.OneToManyPathCalculatorTest#backward_fromNodeD_toNodesBD. It has Ids as class fields, which are reset before the first test runs.

The call hierarchy of Junit5 is:

1. Construct class (create fields)
2. Call AutoReset Extension
3. for each test case:
   a. Call @BeforeEach method
   b. Call test method

[michalmac]

The fixture is a non-static field. That means it is created for each test (6 times in this class).

I don't think so. Because it's a class field, it gets created once. The same problems applies for org.matsim.contrib.dvrp.path.OneToManyPathCalculatorTest#backward_fromNodeD_toNodesBD. It has Ids as class fields, which are reset before the first test runs.

The call hierarchy of Junit5 is:

1. Construct class (create fields)
2. Call AutoReset Extension
3. for each test case:
   a. Call @BeforeEach method
   b. Call test method

Please have a look here: https://junit.org/junit5/docs/current/user-guide/#writing-tests-test-instance-lifecycle.

In order to allow individual test methods to be executed in isolation and to avoid unexpected side effects due to mutable test instance state, JUnit creates a new instance of each test class before executing each test method (see Definitions). This "per-method" test instance lifecycle is the default behavior in JUnit Jupiter and is analogous to all previous versions of JUnit.

[michalmac]

I did now. There are lots of failures. One can reproduce them locally.

I think the problem is that in junit 4, AutoResetIdCaches was a RunListener that was called before a test instance was created.

In junit 5, AutoResetIdCaches is a TestInstancePostProcessor that is called after the instance is created but before the test is run. This corrupts the fields stored in the test instance.

[paulheinr]

Please have a look here: https://junit.org/junit5/docs/current/user-guide/#writing-tests-test-instance-lifecycle.

Thanks for sharing this and clarifying my miss understanding.

I think the problem is that in junit 4, AutoResetIdCaches was a RunListener that was called before a test instance was created.
In junit 5, AutoResetIdCaches is a TestInstancePostProcessor that is called after the instance is created but before the test is run. This corrupts the fields stored in the test instance.

You are right. I switched to TestWatcher instead of TestInstancePostProcessor. That at least removes the failures regarding the ids.

[michalmac]

Super nice to switch to Junit 5. 🚀

I really like that it a full transition (no interim state where we support both). Thanks a lot for making the effort of adapting the test code to the jUnit5 style (e.g. non-public test methods), so when writing new tests we have good patterns to base on. 🥇