Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapt test runner for all JUnit 5 Platform Engines #980

Merged
merged 5 commits into from
May 27, 2020
Merged

Adapt test runner for all JUnit 5 Platform Engines #980

merged 5 commits into from
May 27, 2020

Conversation

jlink
Copy link
Contributor

@jlink jlink commented Apr 21, 2020

This PR wants to tackle
#975

It changes the following things:

  • Mark all methods that have annotation @Testable directly or indirectly present
    Examples are org.junit.jupiter.api.Test or net.jqwik.api.Property
  • Remove restrictions for test method conditions of having return type void because different
    engines handle that differently
  • Mark all inner, non-static classes that have annotation @Testable directly or indirectly
    present as test class. Additionally mark all classes with @org.junit.jupiter.api.Nested as
    a test class.
  • Mark all classes as test classes that have inner test class even if they do not have test
    methods. This is important because a test class might entirely exist of inner test classes.

Current state of implementation

I used test engine https://jqwik.net as example to test the PR. Here's the current state of implementation:

  • Running tests from the editor window of a test class works for both JUnit/Jupiter tests and jqwik tests.
  • Running tests from the hierarchical test runner, though, does not work but outputs:
    No test items found
  • Moreover, trying to runn an inner test class from a jqwik test, fails with
    Error: Failed to parse the JUnit launch arguments.

I definitely need a hint on where to look for fixing these bugs.

@msftclas
Copy link

msftclas commented Apr 21, 2020

CLA assistant check
All CLA requirements met.

@jlink
Copy link
Contributor Author

jlink commented Apr 21, 2020

This is the jqwik test class I've been using to check the test-runner:

package jqwik.example;

import net.jqwik.api.*;

import static org.assertj.core.api.Assertions.*;

@Label("This is a demo")
class Demo1 {

	@Property
	void lengthOfStringAlwaysPositive(@ForAll String aString) {
		assertThat(aString.length()).isGreaterThanOrEqualTo(0);
	}

	@Property
	boolean absoluteValueOfIntegerAlwaysPositive(@ForAll int anInteger) {
		return Math.abs(anInteger) >= 0;
	}

	@Property
	boolean sumOfTwoIntegersAlwaysGreaterThanEach(
			@ForAll int positive1, //
			@ForAll int positive2
	) {
		int sum = positive1 + positive2;
		return sum > positive1 && sum > positive2;
	}

	@Group
	class InnerProperties {
		@Example
		void anEample() {}
	}
}

@jdneo
Copy link
Member

jdneo commented Apr 22, 2020

Thank you @jlink. I'll make sure the PR get reviewed before the end of this Friday.

@jdneo jdneo self-requested a review April 23, 2020 04:55
@jdneo
Copy link
Member

jdneo commented Apr 23, 2020

Running tests from the hierarchical test runner, though, does not work but outputs:
No test items found

This is an existing bug: #773. Root cause is that we are using the search engine to search all the test items but the search engine does not support search the annotation hierarchically. That's why meta annotations are not able to be found.

I think this bug can be addressed in another PR.


Moreover, trying to runn an inner test class from a jqwik test, fails with
Error: Failed to parse the JUnit launch arguments.

I get your point, the error should be throwed here: https://github.com/microsoft/vscode-java-test/blob/master/java-extension/com.microsoft.java.test.plugin/src/main/java/com/microsoft/java/test/plugin/launchers/JUnitLaunchConfigurationDelegate.java#L52-L54

The current implementation swallows it. While the root cause seems that the Eclipse JUnit5 Finder does not find tests in that class, you can step into the method calling to see the details. (I reflect the method calling since it's private in the upstream lib).

@jlink
Copy link
Contributor Author

jlink commented Apr 24, 2020

@jdneo

Running tests from the hierarchical test runner, though, does not work but outputs:
No test items found

This is an existing bug: #773. Root cause is that we are using the search engine to search all the test items but the search engine does not support search the annotation hierarchically. That's why meta annotations are not able to be found.

I'm afraid that real JUnit 5 platform support is not possible by going for annotations at all.
Annotations are just one way to signal that a test (class) is a test (class). Moreover, there is filtering, displaying and conditional execution all built into the platform.
JUnit 5 has a functionality called "discovery" that's handling all that.

If vscode-java-test REALLY wants to support JUnit 5 the maintainers should think about using the the JUnit 5 launcher module which is explicitly made for that purpose. It also supports JUnit 4 by the way. Thus only test-ng would require the legacy way of going through eclipse jdt tooling. But of course that'd be quite a fundamental change.

In case you want to go that route I'd be willing to support you since I know the launcher API quite well - I was in the original team who came up with the design and API.

@jlink
Copy link
Contributor Author

jlink commented Apr 24, 2020

Moreover, trying to runn an inner test class from a jqwik test, fails with
Error: Failed to parse the JUnit launch arguments.

I get your point, the error should be throwed here: https://github.com/microsoft/vscode-java-test/blob/master/java-extension/com.microsoft.java.test.plugin/src/main/java/com/microsoft/java/test/plugin/launchers/JUnitLaunchConfigurationDelegate.java#L52-L54

The current implementation swallows it. While the root cause seems that the Eclipse JUnit5 Finder does not find tests in that class, you can step into the method calling to see the details. (I reflect the method calling since it's private in the upstream lib).

Running JUnit 5 tests should definitely use JUnit 5 launcher module. Adding decorators using annotation matching is a reasonable trade-off since you do not want to compile all code just to decorate elements in an editor. Running is a different beast though, because a lot of things can influence:

  • what is run (think filters, tags, engines)
  • in which order it is run
  • how running results should be displayed/reported
  • displaying other information that can be reported during a test run

As for this PR, I think it's still an improvement for small JUnit 5 exercices but it cannot tackle the real problem. Maybe a reasonable way forward is to split this project into vscode-java-testng and vscode-java-junit.

@jdneo As mentioned in my comment above, I'm willing to help with migration to JUnit launcher. I'd be happy to discuss the JUnit platform approach through video if you are interested.

@jdneo
Copy link
Member

jdneo commented Apr 24, 2020

@jlink Thanks for those information.

I have some questions:

It also supports JUnit 4 by the way

Does that mean that we can use one JUnit 5 to support both JUnit4+5? How about the compatibility for different versions of JUnit 4?

As for this PR, I think it's still an improvement for small JUnit 5 exercises.

Agree, this PR already unblocks a lot of scenarios. I'm willing to merge it. Before that, would you mind to add some client-side test cases? (I'm also okay to do this if you do not want to, just feel free to let me know)

Maybe a reasonable way forward is to split this project into vscode-java-testng and vscode-java-junit

How about providing a setting to turn on/off TestNG support. If it's turned on, we just append some logics to do some tasks like test discoverying

@jlink
Copy link
Contributor Author

jlink commented Apr 24, 2020

@jlink Thanks for those information.

I have some questions:

It also supports JUnit 4 by the way

Does that mean that we can use one JUnit 5 to support both JUnit4+5? How about the compatibility for different versions of JUnit 4?

Yes. There's a dedicated engine called "JUnit Vintage" for exactly that. There's one drawback: It would require to load JUnit platform and Vintage engine under the hood - even if it's not in a project's Gradle/Maven dependencies.

As for this PR, I think it's still an improvement for small JUnit 5 exercises.

Agree, this PR already unblocks a lot of scenarios. I'm willing to merge it. Before that, would you mind to add some client-side test cases? (I'm also okay to do this if you do not want to, just feel free to let me know)

Can you point me the client-side tests for JUnit 4, then I can try.

Maybe a reasonable way forward is to split this project into vscode-java-testng and vscode-java-junit

How about providing a setting to turn on/off TestNG support. If it's turned on, we just append some logics to do some tasks like test discoverying

That's also an option. I figured that splitting would greatly simplify the code base but I may underestimate the amount of shared code.

@jdneo
Copy link
Member

jdneo commented Apr 24, 2020

To add client test contains following steps:

  1. Add a test project in the test project folder. You can add a new project which has some simple cases for jqwik

  2. Add tests in the test folder. You can create a folder for the JUnit 5 and add tests there. Here is an example of the existing JUnit4 cases. You can do the same things -- get the CodeLens and execute them.

  3. Add test calling here. This is to let VS Code execute the cases in a new session.

  4. Add a VS Code launch configuration for debugging purpose.

// By writing TS code, you can install the TS Lint extension to help you solve some linting issues.

@jdneo
Copy link
Member

jdneo commented May 7, 2020

Hey @jlink, anything I can help with the test part? I can add the tests if you are busying with your own work.

@jlink
Copy link
Contributor Author

jlink commented May 7, 2020

@jdneo It’s still on my list but it fell in priority due to other more urgent stuff. So if you have the minutes to spare go ahead and add the tests. It would take some pressure off me.

@jdneo
Copy link
Member

jdneo commented May 7, 2020

Ok, let me add the test cases. But still want to thank you for your contribution!

@jlink
Copy link
Contributor Author

jlink commented May 7, 2020

@jdneo You're welcome! And I'd be happy to help with migrating the extension to full JUnit 5 Platform support.

@jdneo jdneo added this to the 0.23.0 milestone May 27, 2020
@jdneo jdneo merged commit f2c227a into microsoft:master May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants