Provide JUnit 5 equivalent of XpectRunner

[trancexpress]

The JUnit 4 code looks like this:

XpectRunner takes the @XpectTestFiles test DSLs and generates a test case for each DSL test file, i.e. it generates test cases.

XpectFileRunner takes a DSL test file and generates test methods for a test case, these are coming from the XPECT statements (XpectInvocation or XjcTestMethod, I'm not sure which is what in the Xpect tests.

XpectTestRunner generates a test method for a XpectInvocation.

TestRunner generates a test method for a XjmTestMethod.

The code JUnit 4 is "transformed" as follows:

JUnit 4 class	JUnit 5 class
XpectRunner	XpectDynamicTestFactory
XpectFileRunner	XpectDynamicTestCase
XpectTestRunner	XpectInvocationDynamicTest
TestRunner	XpectDynamicTest

There is lots of supporting code that is not JUnit 4 specific, it remains functional and unchanged.

The API proposal is to add a marker interface org.eclipse.xpect.dynamic.IXpectDynamicTestFactory (name TBD), that is implemented by a test case in order to enable Xpect tests (based on JUnit 5) for that test case.

There is also org.eclipse.xpect.dynamic.XpectDynamicTestCase (also name TBD), which has setUp() and tearDown() callbacks, called before reps. after each test method. We require these, since the JUnit 5 construct for generating tests, dynamic tests, doesn't have before/after callbacks (junit-team/junit5#1292 (comment), see also warning here: https://junit.org/junit5/docs/current/user-guide/#writing-tests-dynamic-tests). XpectRunner itself generates tests based on annotation values via JUnit 4 runners. XpectDynamicTestCase can be used by extending it and specifying the annotation @XpectReplace(XpectDynamicTestCase.class) at the extending class. Then importing the extended class at the client test case via e.g.:

...
@XpectImport({MyXpectDynamicTestCase.class ...})
public class MyXpectTestCase implements IXpectDynamicTestFactory {
...

[merks]

FYI, due to certificate signing problems, I highly recommend you avoid doing anything that would kick off build that promotes artifacts. PR builds are fine, but we should avoid "corrupting" the update site with broken signatures.

[trancexpress]

FYI, due to certificate signing problems, I highly recommend you avoid doing anything that would kick off build that promotes artifacts. PR builds are fine, but we should avoid "corrupting" the update site with broken signatures.

@merks you mean artifacts that would result from needing to build Xpect if this PR gets merged? No problem, the change is not high priority.

Or just the PR itself results in built artifacts due to the jenkins checks? In that case I should generally wait before force pushing?

[trancexpress]

org.eclipse.xpect.xtext.lib.tests defines a bunch of test bases (I assume), that are annotated with @RunWith(XpectRunner.class).

At Advantest, those are not extended from, so no interest from my side to provide JUnit 5 alternatives. Clients can define a base, that extends the old base, but also implements IXpectDynamicTestFactory. Or whatever we end up calling the new marker interface, if it survives in this form. That is enough for it to work.

[merks]

The PR builds are fine. They don't even do signing and definitely don't publish results. Until this is resolved

https://www.eclipsestatus.io/incident/373129

It's best not to do any non-PR builds.

[trancexpress]

I'm going over Advantest tests with the suggested changes. There are hiccups, but generally tests run. Tests also fail if I change their expectations.

So I assume the changes here are on the right track.

@tjeske FYI.

[trancexpress]

A further issue I see is how setup validation and state clean-up is done by the current XpectFileRunner.

Dynamic tests have no support for such things, see e.g.:

junit-team/junit5#1292 (comment)
https://junit.org/junit5/docs/current/user-guide/#writing-tests-dynamic-tests (the warning block at the end)

2 options I see are:

• Add a test before each test in the file, that does setup validation. Add a test after the tests, that does state clean-up.
• First test does also setup validation. Last test does clean-up.

Both have their problems, in particular relying on test execution order (which I hope is well defined for dynamic tests, or there is a way to make it well defined). As well as tests running after the setup validation failed...

[trancexpress]

@iloveeclipse if you can review the code, it wouldn't hurt. If you have time we can go over the code in a call, that might simplify the review.

For the most part, the new code is copy-pasted from the JUnit 4 code, replacing the JUnit 4 specific things and keeping the rest as is. Though identifying what is copy and what isn't won't be fast if you have not seen the JUnit 4 code yet.

Anyway, essentially the code is "transformed" as follows:

JUnit 4 class	JUnit 5 class	what it does
XpectRunner	XpectDynamicTestFactory	takes the @XpectTestFiles test DSLs and generates a test case for each DSL test file, i.e. it generates test cases
XpectFileRunner	XpectDynamicTestCase	takes a DSL test file and generates test methods for a test case, these are coming from the XPECT statements (XpectInvocation or XjcTestMethod, I'm not sure which is what in the Xpect tests
XpectTestRunner	XpectInvocationDynamicTest	this generates a test method for a XpectInvocation
TestRunner	XpectDynamicTest	this generates a test method for a XjmTestMethod

The names are up to discussion, same as with the new API - implementing the marker interface , in order to get Xpect tests. And how the set up and tear down is delegated to the client, via XpectDynamicTestCase.setUp() resp. XpectDynamicTestCase.tearDown().

Our tests work fine with this change, aside from three (out of 37) test cases (that don't seem to do anything on JUnit 5 but runs a few tests on JUnit 4) and the mentioned handling of the java_test project resulting in logged errors that we fail to ignore with JUnit 5. I'm looking into both, but I doubt anything major needs to change in this PR; so it can be reviewed now.

[trancexpress]

I wonder if we should drop the setup and teardown logic and just let and extending class override the execute of each dynamic test. That way the client code can do whatever they want around the test execution and we are not adding weird API.

[trancexpress]

As a crutch, XpectRunner can be instantiated in whatever class is supposed to replace it (when generating dynamic tests with JUnit 5). Its model has to be re-used, the test class needs to be passed to it. This is to avoid the need for #344. It of course means keeping the JUnit 4 dependency, as the code will not run without it.

But technically its possible to define all the replacement parts completely out of Xpect, without changes, so that Xpect tests run on JUnit 5. Essentially just take the classes from the dynamic package (found in this PR) in your on code base, making sure to properly handle the singleton aspect of XpectRunner.

I was able to do so in the product I work on.

Of course, ideally we propagate some version of this PR to Xpect, to avoid the need for clients to do all this work. But API is difficult to define, so it might take a while - considering that so far there is no feedback from Xpect maintainers.

[iloveeclipse]

Hint: all new classes miss file headers

[iloveeclipse]

Please use blocks for if

[iloveeclipse]

The change looks OK for me, just file headers should be added.

Class names also make sense for me (which is the point where we usually have different opinions with Simeon), but this time I was able to easily identify which class does what and that is a good sign.

Note: I have not much knowledge in Xpect except that I own our small custom test extension to it that is used by many our internal xpect based tests.

With that PR we are able to convert our Xpect tests from JUnit 4 to JUNit 5 with relatively low effort on the test "glue code" and zero effort on the many actual test cases.

[iloveeclipse]

@tjeske : would be nice if you could look at that PR.

[trancexpress]

@tjeske : would be nice if you could look at that PR.

@tjeske if you don't have much time, but do have some, please check #344 with priority. It adjusts what we need to adjust in Xpect to (hopefully) get rid of JUnit 4.

The PR here is of course also nice to have, but not a must from our POV - anyone can copy paste the code from it and use it in their project, instead of XpectRunner.

[tjeske]

Hey guys, sorry for the delay. I had some days off.

@trancexpress: Thanks for your contribution! I have a few questions...
What was the reason why you create tests dynamically? Why not having a JUnit5 extension?
Can we please have a JUnit5 test added?

[trancexpress]

What was the reason why you create tests dynamically? Why not having a JUnit5 extension?

XpectRunner itself creates tests on-the-fly, based on values in annotations (I think XpectFiles or something like that). What kind of annotation can achieve this? I thought only dynamic tests can generate tests in JUnit 5.

Can we please have a JUnit5 test added?

You mean test the new functionality? @iloveeclipse do you want to invest time in this? I'm not sure if there are actual tests for XpectRunner, other than tests in Xpect that rely on it.

For us the test was to replace our XpectRunner tests; I have compared tests before and after, the results are as expected. Having to add tests in Xpect / move Xpect tests to JUnit 5 / discuss API is just more reasons to add the code in our code base and avoid investing the remaining effort that goes into public APIs of bundles we consume.

[iloveeclipse]

Not sure if there are some Xpect tests that could be moved to JUnit 5 as a "test"? May be as extra testsuite?
Writing dedicated test for XpectRunner is a bit too much effort for now.

[trancexpress]

Not sure if there are some Xpect tests that could be moved to JUnit 5 as a "test"? May be as extra testsuite?

There are. I guess we can partially move some to the new construct. Though no idea how much effort will be needed to get that running - no idea if any JUnit 5 tests exist yet in Xpect.

[tjeske]

I think we should add some testing for the new functionality if we want to integrate this PR. The other PR LGTM (as far as I can tell).

[trancexpress]

I think we should add some testing for the new functionality if we want to integrate this PR. The other PR LGTM (as far as I can tell).

Alright, lets continue with #344 for now.

[cdietrich]

maybe we can have the same test set run in 4 and 5
a least for a subset

[trancexpress]

maybe we can have the same test set run in 4 and 5 a least for a subset

I think the JUnit 4 and JUnit 5 variants for Xpect testing are compatible with each other, we must add the new interface to test cases that use XpectRunner.

What is the JUnit 5 status in Xpect? Is it used? Is there some separation of JUnit 4 tests and JUnit 5 tests, or is everything running on JUnit 5 with the vintage engine? I've not looked into it, I guess the next stop here is to do that.

[cdietrich]

I am not aware of any junit 5 use. The code is 10 years and older