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

JUnit testing support Bug fixes and Enhancements - Step 1 #4303

Merged
merged 22 commits into from
Mar 10, 2017

Conversation

davidfestal
Copy link
Contributor

@davidfestal davidfestal commented Mar 3, 2017

What does this PR do?

This PR provides a number of bug fixes / enhancements to the current limited JUnit testing support in Che.

More precisely:

  • it provides a new Tests text console view to follow in real-time the tests being run
  • it fixes problems encountered when trying to run tests on vertx projects:
    • during the test session the working directory should be set to the project base directory, to be ensure that write access in the current directory is provided to the tested code
    • during the test session the thread context class-loader should be set to the class-loader used to load / run the tested classes (which includes the tested project classpath).
  • it reduces the test execution time, by removing the need of running a "maven dependencies" command to build the project test classpath. Instead of that it uses the ClasspathService to retrieve the already-built classpath.

What issues does this PR fix or reference?

https://issues.jboss.org/browse/CHE-111
https://issues.jboss.org/browse/CHE-121

Changelog

JUnit testing support Bug fixes and Enhancements

  • Provided a new Tests text console view.
  • Fixed problems encountered when trying to run tests that:
    • Require write access in the current directory.
    • Use the thread context class-loader during test execution.
  • Reduced the JUnit test execution time by removing an unnecessary step.

Release Notes

(Include in release notes only if other PRs are merged in 5.5)

Important notice: This PR is the first step of series of PR that will all contribute to enhance the testing support. Fro all these steps there will probably be a single release note that would look like this:

Testing support enhancements:

  • Real-time notification of test execution,
  • Better view of the successful tests,
  • Navigation from the test view to the source code,
  • various bug fixes

(JUnit 4 `RunListener` is a class, not an interface, so that and simple
`Proxy` cannot be used.

- Set the thread context classloader to the project classloader during
the time of the test (required for VertX support for example)

- Add test session start and stop events in the `AbstractTestListener`

- Clear the `Tests` output view when a new test is started 


Signed-off-by: David Festal <[email protected]>
... to retrieve the Java project classpath.

Signed-off-by: David Festal <[email protected]>
This avoids blinking each time a line is added.

Signed-off-by: David Festal <[email protected]>
This is necessary for all the tests that, usually started with `mvn`,
assume they are started in the project root directory, and have all
write permissions on the current directory.

Without this commit Vertx tests fail with an `IllegateStateException`
because Vertx cannot create its cache directory in the current working
directory (see https://github.com/mlabuda/vertx-with-che for a vertX
test project).

Signed-off-by: David Festal <[email protected]>
in order to build the classpath required to run tests.

This is much faster. 

Signed-off-by: David Festal <[email protected]>
@codenvy-ci
Copy link

Can one of the admins verify this patch?

@@ -33,6 +33,10 @@
<groupId>javax.inject</groupId>
<artifactId>javax.inject</artifactId>
</dependency>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sort pom

@@ -57,5 +61,23 @@
<groupId>org.eclipse.che.plugin</groupId>
<artifactId>che-plugin-testing-classpath-server</artifactId>
</dependency>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sort pom

return false;
}
});
Class<?> c = f.createClass();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format class

return false;
}
});
Class<?> c = f.createClass();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format class

getOrCreateTestSummary(testKey).addError();
addedError(testKey, throwable);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line

consumer.consume(DtoFactory.cloneDto(new TestingOutputImpl(line, lineType)));
} catch (IOException e) {
// TODO Auto-generated catch block
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOG.error?

<dependency>
<groupId>org.eclipse.che.plugin</groupId>
<artifactId>che-plugin-machine-ext-client</artifactId>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sort pom

@@ -37,5 +37,17 @@
<directory>src/main/java</directory>
</resource>
</resources>
<plugins>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sort pom

@@ -111,6 +111,15 @@
</resource>
</resources>
<plugins>
<plugin>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sort pom

@benoitf
Copy link
Contributor

benoitf commented Mar 3, 2017

ci-build

return toResolvedClassPath(classpathService.getClasspath(relativeProject).stream())
.map(dto -> {
try {
String dtoPath = dto.getPath();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems there is usage of tab characters and indent is not correct

this.lineType = lineType;
}

private String output;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting looks odd

@davidfestal
Copy link
Contributor Author

Thanks for these comments @benoitf @skabashnyuk. And sorry for all the formatting problems, that probably mostly come from some bad tab/space eclipse settings. I'll fix all this quickly.

@benoitf
Copy link
Contributor

benoitf commented Mar 3, 2017

@davidfestal thx

@codenvy-ci
Copy link

Build finished.
Build success. $BUILD_URL

@codenvy-ci
Copy link

dfestal added 3 commits March 3, 2017 16:19
Signed-off-by: David Festal <[email protected]>
Signed-off-by: David Festal <[email protected]>
@davidfestal
Copy link
Contributor Author

@benoitf @skabashnyuk I fixed the requested changes.


@Inject
public MavenTestClasspathProvider(ClasspathService classpathService) {
this.classpathService = classpathService;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidfestal I still see here a tab character ?

private Stream<ClasspathEntryDto> toResolvedClassPath(Stream<ClasspathEntryDto> rawClasspath) {
return rawClasspath.flatMap(dto -> {
if (dto.getEntryKind() == IClasspathEntry.CPE_CONTAINER) {
return toResolvedClassPath(dto.getExpandedEntries().stream());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and there. so I'm not sure it's up-to-date ?

Signed-off-by: David Festal <[email protected]>
@davidfestal
Copy link
Contributor Author

@benoitf : this time there shouldn't be any tab any more in all the projects I touched !

private final TestServiceClient service;
private final AppContext appContext;
private final SelectionAgent selectionAgent;
private RunTestActionDelegate delegate;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason why this delegate is not a final variable ?

private final TestResultPresenter presenter;
private final TestServiceClient service;
private final TestServiceClient service;
private RunTestActionDelegate delegate;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question there

}

public RunTestActionDelegate(
Source source) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's the formatter that has done this newline break ?

<artifactId>license-maven-plugin</artifactId>
<configuration>
<excludes>
<exclude>**/**/AbstractTestListener.java</exclude>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we skip these files from the license plugin ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the new files I created, I added the copyright in the name of RedHat, Inc, and the license-checking maven plugin doesn't like that.

@skabashnyuk
Copy link
Contributor

I don't know. May be @davidfestal can explain.

@bmicklea bmicklea added the kind/enhancement A feature request - must adhere to the feature request template. label Mar 4, 2017
@bmicklea
Copy link

bmicklea commented Mar 4, 2017

@davidfestal do you expect the other PRs will be ready in the next week? Or will you need more time?

@davidfestal
Copy link
Contributor Author

The idea is to have most of the other PRs ready at the end of next week yes.

@TylerJewell
Copy link

@davidfestal - it would be nice to be merge-ready by Thursday COB. We try to close the release every Wednesday, but over the past two releases we have slipped 1-2 days as there are last minute PRs that merge on Monday or Tuesday, and then break the builds, forcing another QA cycle.

A Thursday night merge gives plenty of time to resolve any outstanding issues in time for the following Wednesday release.

@davidfestal
Copy link
Contributor Author

I understand the idea @TylerJewell. So in this case, I assume I could simply add my next commits to this PR until Thursday night, so that they can be reviewed as soon as they are added, and we will be ready to merge on Friday ?

@TylerJewell
Copy link

Yes. Or if you miss the 5.5 release window, you just merge into the next cycle and it will be part of 5.6 two weeks later.

... sending the build output to the a command view, and execute the
tests only if compilation succeeded.

Signed-off-by: David Festal <[email protected]>
This includes some utility mocking classes that could be useful in other
contexts. 

Signed-off-by: David Festal <[email protected]>
Copy link
Contributor

@skabashnyuk skabashnyuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidfestal any plans to finalize this PR so we will be able to made final review?

/**
* Service for getting information about classpath.
*
* @author Valeriy Svydenko
*/
@Path("java/classpath/")
public class ClasspathService {
public class ClasspathService implements ClasspathServiceInterface {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of interface for rest service with such weird name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ClasspathService component is now also used from the server-side class org.eclipse.che.plugin.testing.classpath.maven.server.MavenTestClasspathProvider.

And when implementing the test for MavenTestClasspathProvider, mocking ClasspathService directly brought classpath problems, while mocking the implemented interface works perfectly.

see https://github.com/davidfestal/che/blob/5a698ac9aed88f54c6eb98372db493ef39b3ec8b/plugins/plugin-testing-java/plugin-testing-classpath/che-plugin-testing-classpath-maven-server/src/test/java/org/eclipse/che/plugin/testing/classpath/maven/server/MavenTestClasspathProviderTest.java#L46

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you've added this interface for testing purposes.
From our experience this approche has some disadvantages.

  1. Inheritance in Rest services is quite unusual and we are trying to avoid it in our code.
  2. Name of interface is syntetic, it show that it was made for some reason, but not for real inheritance.
  3. Mockitng of rest service is quite rare too.

If you do realy want to test Rest service I recomend you to take a look on @listeners(value = {EverrestJetty.class, MockitoTestNGListener.class})
For example
https://github.com/eclipse/che/blob/master/wsmaster/che-core-api-user/src/test/java/org/eclipse/che/api/user/server/UserServiceTest.java
https://github.com/eclipse/che/blob/master/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceServiceTest.java

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I added this for testing purposes. The fact is that this rest service was not used by sever-side components until now. It was only used through ClasspathServiceClient which has not exactly the same method signature (it returns a client promise).

So probably there should be a singleton class, say ClasspathProvider available for injection in server-side components, and used by the ClasspathService rest service.

So do you want me to work on removing ClasspathServiceInterface now in this PR, or can it wait for the next PR ?
In other words does it prevent you to approve the merge of this PR or not ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I do want to remove ClasspathServiceInterface. But if you think it'll take significant time you can do that in next PR.

ArgumentType arg = (ArgumentType)invocation.getArguments()[0];
return apply.apply(arg);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at the end of the file ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, Github has nice icon to show that,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line added

}
}).filter(field -> field != null).toArray(Object[]::new));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at the end of the file ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line added

* @author David Festal
*/
public interface MockitoPrinter {
default void printInvocationsOnAllMockedFields() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this interface look wired. How it will be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interface is used this way:

public class TestClass implements MockitoPrinter {
  @Mock private aMock1; 
  @Mock private aMock2;
   ...

    @Test
    public void testFunction() {
        ....
        // print all mock invocations called during the test on 
        // mocked fieldsfor debugging and test design purposes  
        printInvocationsOnAllMockedFields();
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I've missed the fact that is part of the tests

}));
return this;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at the end of the file ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line added

@davidfestal
Copy link
Contributor Author

@davidfestal any plans to finalize this PR so we will be able to made final review?

@skabashnyuk: I plan to further push additional commits very soon, but since this PR is already a consistent whole, if you want I can also push next commits on a separate branch to produce a separate PR.
This way you would all be able to review this PR first.

Copy link
Contributor

@skabashnyuk skabashnyuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to make a review before merge.
If you are going to add more and more changes I think it would be reasonable to merge Step1 to master, and after create new pr for Step 2 . WDYT?

@davidfestal
Copy link
Contributor Author

@skabashnyuk I'm OK for merging the PR now and pushing next commits in PR Step 2, sure.
Is there anything you want me to change before merging this PR (see this comment)?

Copy link
Contributor

@skabashnyuk skabashnyuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We good to go.

@benoitf
Copy link
Contributor

benoitf commented Mar 10, 2017

For merge, please use squash option

@l0rd l0rd merged commit 440f92d into eclipse-che:master Mar 10, 2017
@l0rd
Copy link
Contributor

l0rd commented Mar 10, 2017

@davidfestal build failed please have a look https://ci.codenvycorp.com/job/che-ci-master/1069/

@davidfestal
Copy link
Contributor Author

davidfestal commented Mar 10, 2017

Yes, it's a problem of undeclared POM transitive dependencies, that make the maven dependency analysis fail.
I already have the fix ready here but I'm waiting for the end of a local full build before submitting it in a new PR.

@davidfestal
Copy link
Contributor Author

The PR is submitted: #4391

@JamesDrummond JamesDrummond mentioned this pull request Mar 17, 2017
9 tasks
JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017
…e#4303)

Provide  JUnit test real-time output in a `Tests` output view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants