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

chore(junit5): ports tests from arquillian-junit-container #313

Merged
merged 10 commits into from
Feb 23, 2021

Conversation

kifj
Copy link
Contributor

@kifj kifj commented Feb 14, 2021

After merging the PR for #301 there is a new issue as described in #309. This could have been avoided if we had (more) tests in the new junit5 module.

With this PR I tried to convert the existing tests for JUnit4 in arquillian-junit-container to arquillian-junit5-container. I'm not sure if they are all useful, and I needed to adapt and delete some. But at least the issue with the lifecycle methods is detected by a test failure, and the fix provided by @lprimak in #310 makes them green again.

@kifj
Copy link
Contributor Author

kifj commented Feb 14, 2021

The failing test in CircleCI is to be expected as long as #310 is not fixed

@lprimak
Copy link
Contributor

lprimak commented Feb 14, 2021

That's fantastic @bartoszmajsak

lprimak
lprimak previously approved these changes Feb 23, 2021
Copy link
Member

@bartoszmajsak bartoszmajsak left a comment

Choose a reason for hiding this comment

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

Thank you very much for this effort! Much appreciated.

I have few minor suggestions around license headers which you can apply automatically and one remark about test method name :)

public class JUnitIntegrationTestCase extends JUnitTestBaseClass {

@Test
public void should_execute_extensions() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

As much as I'm in favor of underscores in the test method names I think we should play a bit conservative here and stick to the convention we have across the project. OR... rename everything to underscores - your call ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not in favor either and will stick to the conventions - names are copies of the junit4 tests.
I will rename all test methods to camelCase.

Copy link
Member

Choose a reason for hiding this comment

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

If that's the case I will review the rest and take care of boy scouting!

kifj and others added 7 commits February 23, 2021 10:37
…tainer/JUnitJupiterDeploymentAppenderTestCase.java

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>
…tainer/JUnitJupiterTestRunnerTestCase.java

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>
…tainer/JUnitTestBaseClass.java

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>
…tainer/ClassWithArquillianExtensionWithExtensions.java

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>
…tainer/TestScenarios.java

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>
…tainer/JUnitIntegrationTestCase.java

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>
@lprimak
Copy link
Contributor

lprimak commented Feb 23, 2021

@kifj I think you need to force-push, this is not a "real" failure

Copy link
Member

@bartoszmajsak bartoszmajsak left a comment

Choose a reason for hiding this comment

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

Thank you!

@bartoszmajsak bartoszmajsak changed the title Converting tests from arquillian-junit-container chore(junit5): ports tests from arquillian-junit-container Feb 23, 2021
@bartoszmajsak bartoszmajsak merged commit 7e2350d into arquillian:master Feb 23, 2021
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