Migrate to JUnit5

[yb172]

Update all tests to use JUnit5 instead of JUnit4 based on discussion in #506.

At least two things would have to be done:

• Remove dependency on junit4 and move all tests to junit5
• Refactor EtcdClusterResource to be JUnit5 extension instead of JUnit4 ExternalResource

[yb172]

@fanminshi @lburgazzoli I've migrated tests to JUnit5 in #547

A few notes:

• Moving @BeforeClass, @AfterClass, @Before and @After, and @Test was trivial
• Moving to @ParametrizedTest was not very trivial but ok. Code seem to be a bit more concise.
• There are not many examples on JUnit5. It took a while to figure out how to wire EtcdClusterExtension to a field. Had to look at third-party extensions source.
• Extensions state is very confusing. Some articles are saying that extension must be stateless so it's not clear if provided way of doing EtcdClusterExtension is correct. It looks ok, but it's not clear if that's the case. And code to work with extension store is super cumbersome.
• There is no support for proper test-level timeouts (feature request was opened in 2015 and is still open) which results in assertTimeoutPreemptively for every test: https://github.com/etcd-io/jetcd/pull/547/files#diff-1f8fc953ba87e8787c95a7605b2d7796R85. And assertTimeoutPreemptively could result in side-effects.
• Some tests have external @Rules so we can't completely move to JUnit5 and have to use @EnableRuleMigrationSupport which is experimental. And it doesn't look that GrpcServerRule would be migrated to extension in nearest future.

Given absence of global timeout support and that we would have to keep junit dependency anyway to support @Rule I'm not sure if #547 should be merged.

What do you think?

[dbyron0]

This is a long shot, but by any chance would @marcphilipp mind taking a look at this and help point us in the right direction? You've been super helpful with junit info elsewhere (e.g. in the gradle repo) so I figured I'd give it a whirl here too. Thanks much.

[fanminshi]

Some tests have external @Rules so we can't completely move to JUnit5 and have to use @EnableRuleMigrationSupport which is experimental. And it doesn't look that GrpcServerRule would be migrated to extension in nearest future.

I wrote the those unit tests way before we had the test-container capability. I think in the future, we can just write integration tests. I don't see too much utility of writing unit tests beside using a stub grpc server to return specific errors. I think those can be removed.

There is no support for proper test-level timeout
That't a good finding! I don't like the assertTimeoutPreemptively to be added in every test. it is too verbose. Is it possible to use the junit4 timeout rule within a junit5 test? Alternatively, we don't require timeout for each test. We can configure a global suite timeout at travis level. By default, any job doesn't finish within 50 minutes will be killed. ref: https://docs.travis-ci.com/user/customizing-the-build/#build-timeouts and we can configure those timeout seen from https://stackoverflow.com/questions/29836823/how-to-increase-timeout-in-travis-ci-for-my-selenium-tests-written-on-java.

Overall, our integration tests should complete in a reasonable time. I can see the test level timeout in debugging hanging tests. However, I don't think that's a strict requirement. I think it is fine we can remove the restriction on test timeout for now until junit5 add those feature. Then we can revisit this problem.

[marcphilipp]

Extensions state is very confusing. Some articles are saying that extension must be stateless...

It should be stateless because the same extension instance might be used concurrently when using parallel execution for different test methods or even classes (if the extension is registered globally). Thus, I'd recommend storing state in the store. There's a bit of a learning curve to it, let me know if you have questions. If we can improve the docs based on your feedback, that'd be great.

There is no support for proper test-level timeouts

Test-level timeouts are on the agenda for JUnit 5.5.

@dbyron0 Please let me know how I can help. 🙂

[yb172]

@marcphilipp does it mean that this implementation is incorrect even if it's used with @RegisterExtension (example)?

Thanks for letting us know about timeouts!

[marcphilipp]

It will work as long as you don't use @Nested classes.

[dbyron0]

Thank you @marcphilipp !

[lrozenblyum]

Hi @dbyron0, @marcphilipp.
Sorry for bring up a pretty old thread.
I'm facing a similar challenge in migrating from JUnit4 to JUnit5 in a private repository.

We have a JUnit4 @Rule that provides an accessor to change its private field data during execution (the accessor was called from the test itself), so its proper work was based on the stateful behavior.

Now we're going to migrate it to an Extension and use it via @RegisterExtension.

So the question is: does this way of usage of an extension requires making it stateless?
The official JUnit5 doc doesn't seem mention it explicitly although some internet articles mention this requirement (like this one https://nipafx.dev/junit-5-extension-model/#Stateless).

The paragraph https://junit.org/junit5/docs/current/user-guide/#extensions-keeping-state mentions need to use the ExtensionContext but this doesn't seem a feasible option to use from the test itself.

Also it mentions Usually, an extension is instantiated only once. Is it some guarantee from the library? In this case, the state should be preserved and no problems about keeping something in the extension's fields?

Thanks in advance for clarifying this!

[marcphilipp]

It's still a good practice to manage state via ExtensionContext because that makes it more versatile. If you make it stateful, you should only use it via @RegisterExtension which instantiates it per test class.

[lrozenblyum]

Thanks for clarification @marcphilipp!