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

Add basic integration test for JUnit 5 #118

Closed
wants to merge 8 commits into from

Conversation

britter
Copy link
Member

@britter britter commented Sep 6, 2016

This is WIP. My take on adding a first basic integration test for JUnit 5. If this is going in the right direction we can add more tests.

</properties>

<dependencies>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the first IT.
I should study what these dependencies mean :)

@Tibor17
Copy link
Contributor

Tibor17 commented Sep 6, 2016

@brianf
In generally this is a good starting point of course.
We should have such ITs which cover the functionality of provider from the configuration point of view. So as for instance you can see in the old ITs, we can test that re-run works or parallel execution works or forking JVMs, or XML report is complete, groups/categories/tags, fail-fast feature or shutdown works, additional listeners, or combination of JUnit 5 and TestNG.
Then failure/erros/flakes/skipped are reported in console and XML report so that the listener works fine.
People use Filters (miscellaneous combination of class & method patterns) test/includes/excludes, and some people use in-plugin execution and many use forks.
It's maybe worth to write Parameterized IT which alters the tests via using profiles and so the same POM can be used with different configs but the test result is very the same - depends on requirements. The parameterized test may save our time so that we write the Base test once and parameters just easily added - see such in the old tests, it maybe helps.

@Tibor17
Copy link
Contributor

Tibor17 commented Sep 6, 2016

I did not notice you was @ IRC. The build takes 40 minutes to complete :) We have many tests :)
If I add an IT only then it's worth to select only particular class name of the IT in config of maven-failsafe-plugin of POM maven-surefire-integration-tests.

@britter
Copy link
Member Author

britter commented Sep 7, 2016

@Tibor17 thank you for the input. I'd like to take have small PRs, so my proposal is to introduce this first PR and merge it to the code base after your comments have been resolved and then work incrementally on more tests which cover the different areas of configuration, etc. WDYT?

@britter
Copy link
Member Author

britter commented Sep 7, 2016

@Tibor17 worked through all your comments. Can this be merged as a first starter for JUnit 5 ITs?

@Tibor17
Copy link
Contributor

Tibor17 commented Sep 8, 2016

We cannot really marge the external provider to master if it is alpha or
beta version and if we do not have ITs.
We in hurry too much however we take full responsibility for the quality.
It's better to create a branch and update the branch. What we are missing
is documentation in Surefire on how the JUnit 5 is used in POM and test
coverage for the features matrix. This is what the users need to see. Once
we do it the users would like to try it and start reporting bugs against
us. Are you ready for handling responsibility for these bugs,
implementation and JUnit 5 altogether.

On Wed, Sep 7, 2016 at 8:41 PM, Benedikt Ritter notifications@github.com
wrote:

@Tibor17 https://github.com/Tibor17 thank you for the input. I'd like
to take have small PRs, so my proposal is to introduce this first PR and
merge it to the code base after your comments have been resolved and then
work incrementally on more tests which cover the different areas of
configuration, etc. WDYT?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#118 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA_yRzQexXIQAijPOLHh9NkShzd7Gxe9ks5qnwVfgaJpZM4J2QAo
.

Cheers
Tibor

@britter
Copy link
Member Author

britter commented Sep 8, 2016

@tibor this PR is only about introducing ITs. The M2 release of the provider is pulled in via maven as dependency, see the test pom.xml. I agree that surefire should not ship with a have finished JUnit 5 provider. But since this is just adding tests against an already released artifact, I don't see a reason not to merge it. We can extend the tests against milestone releases from the JUnit team and extend the test coverage. When we have a decent coverage we can move their provider implementation to surefire. The integration of the provider should then be done in a separate branch.

@Tibor17
Copy link
Contributor

Tibor17 commented Sep 8, 2016

One more job to do in *IT.java. Can you please add
assumeThat(java.specification.version, is(greaterThan(1.7)));.
A hint: Just lookup the text "java.specification.version" in ITs.
We use similar treatment for ITs using JDK7 and another ITs in
maven-failsafe-plugin as well.

I will call a vote for release soon and it is better really to not let PMC
see that build failed with JDK6.
The community starts the build by hands in the Vote and in my experience
they always had question and sometimes repeating still the same.

Do you have permissions to login to our Jenkins [1]?
Try to change the JDK without your commit yet and wait for the build. It
should not fail.
One the web page you will see Surefire job for Windows but not sure if JDK
8 can be there. Do not worry if this Win build is yellow because masine is
very slow (maybe GC) and use to fail the performance tests with JUnit7
parallel exec.
[1] https://builds.apache.org/job/maven-surefire/

If you want to refactor the test, try to do it before git push.
Yes I think it does not harm the project but please wait for the answer
from Jenkins.

Back to running the build on your laptop.
I do not know about your PC but mine spends 46 minutes for complete build
(mvn -Prun-its install).
It is Intel Core i7 3.00 GHz, 8 GHz using SSD. The hard drive really
matters, SSD better over spinning drives.

On Thu, Sep 8, 2016 at 10:59 AM, Benedikt Ritter notifications@github.com
wrote:

@tibor https://github.com/tibor this PR is only about introducing ITs.
The M2 release of the provider is pulled in via maven as dependency, see
the test pom.xml. I agree that surefire should not ship with a have
finished JUnit 5 provider. But since this is just adding tests against an
already released artifact, I don't see a reason not to merge it. We can
extend the tests against milestone releases from the JUnit team and extend
the test coverage. When we have a decent coverage we can move their
provider implementation to surefire. The integration of the provider should
then be done in a separate branch.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#118 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA_yR4X2xWe3kXTX66OVOW_ZFWUu4b1Oks5qn85ngaJpZM4J2QAo
.

Cheers
Tibor

@@ -53,13 +53,13 @@ public void tearDown()
@Test
public void testSetUp()
{
assertTrue( setUpCalled, "setUp was not called" );
Assertions.assertTrue( setUpCalled, "setUp was not called" );
Copy link
Contributor

@Tibor17 Tibor17 Sep 8, 2016

Choose a reason for hiding this comment

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

It's up to your taste:)
I like using static import because especially in the assertion statements are verbose and if you read them in whole line then it sounds like sentence you read in news, very narrative. And therefore the class name does not have meaning to make it more verbose, it's just only a place where the function is stored.
The line width is limited to 120 chars which forces me to decide on how to type complete statement without breaking the line and without wrapping with nested call.

@Tibor17
Copy link
Contributor

Tibor17 commented Sep 11, 2016

@britter
I see the CI https://builds.apache.org/job/maven-surefire/ already has JDK8.
👍 after adding assumeThat(java.specification.version, is(greaterThan(1.7))); from #120
This order of pushed PRs was your plan ?

@britter
Copy link
Member Author

britter commented Sep 12, 2016

@Tibor17 yes I need #120 to finish this PR.

@britter
Copy link
Member Author

britter commented Sep 19, 2016

@Tibor17 I've assed the assume statement using the API introduced in #120. I propose that you create a new branch for JUnit 5, where we can merge this PR and all follow up PRs for JUnit 5 integration.

@britter
Copy link
Member Author

britter commented Sep 23, 2016

@Tibor17 any chance this can land on it's own feature branch, so that I can start with the next tests? I don't want to bloat this PR.

@Tibor17
Copy link
Contributor

Tibor17 commented Sep 23, 2016

@britter Sorry I was busy this week. I am going to push this branch.

@Tibor17
Copy link
Contributor

Tibor17 commented Sep 23, 2016

@britter The branch is ready for use.

@britter
Copy link
Member Author

britter commented Sep 24, 2016

@Tibor17 Thank you! I'll continue with the next more complex examples.

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.

2 participants