-
Notifications
You must be signed in to change notification settings - Fork 541
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
Conversation
</properties> | ||
|
||
<dependencies> | ||
<dependency> |
There was a problem hiding this comment.
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 :)
@brianf |
I did not notice you was @ IRC. The build takes 40 minutes to complete :) We have many tests :) |
@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? |
@Tibor17 worked through all your comments. Can this be merged as a first starter for JUnit 5 ITs? |
We cannot really marge the external provider to master if it is alpha or On Wed, Sep 7, 2016 at 8:41 PM, Benedikt Ritter notifications@github.com
Cheers |
@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. |
One more job to do in *IT.java. Can you please add I will call a vote for release soon and it is better really to not let PMC Do you have permissions to login to our Jenkins [1]? If you want to refactor the test, try to do it before git push. Back to running the build on your laptop. On Thu, Sep 8, 2016 at 10:59 AM, Benedikt Ritter notifications@github.com
Cheers |
@@ -53,13 +53,13 @@ public void tearDown() | |||
@Test | |||
public void testSetUp() | |||
{ | |||
assertTrue( setUpCalled, "setUp was not called" ); | |||
Assertions.assertTrue( setUpCalled, "setUp was not called" ); |
There was a problem hiding this comment.
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.
@britter |
@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. |
@britter Sorry I was busy this week. I am going to push this branch. |
@britter The branch is ready for use. |
@Tibor17 Thank you! I'll continue with the next more complex examples. |
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.