[JBWS-4412] Migrate junit4 to junit5 #378
Conversation
|
@jimma would you please review ? thanks |
|
@gaol Thanks for the great contribution! This is really huge improvement to update to junit5, eliminate couple of old TestRules and get the tests more clean and organized. Just couple of minor things, please see my comments inlined. |
...est/java/org/jboss/test/ws/jaxws/cxf/clientConfig/CXFDefaultClientConfigurationTestCase.java
Show resolved
Hide resolved
...te/cxf-tests/src/test/java/org/jboss/test/ws/jaxws/cxf/jbws3655/EarSchemaImportTestCase.java
Show resolved
Hide resolved
...e/cxf-tests/src/test/java/org/jboss/test/ws/jaxws/cxf/noIntegration/EmbeddedCXFTestCase.java
Outdated
Show resolved
Hide resolved
...uite/cxf-tests/src/test/java/org/jboss/test/ws/jaxws/cxf/spring/ClientSpringAppTestCase.java
Outdated
Show resolved
Hide resolved
...xf-tests/src/test/java/org/jboss/test/ws/jaxws/cxf/spring/ClientSpringAppTestCaseForked.java
Outdated
Show resolved
Hide resolved
...a/org/jboss/test/ws/jaxws/samples/wsse/policy/oasis/WSSecurityPolicyExamples23xTestCase.java
Show resolved
Hide resolved
...ed-tests/src/test/java/org/jboss/test/ws/jaxws/clientConfig/ClientConfigurationTestCase.java
Show resolved
Hide resolved
.../testsuite/shared-tests/src/test/java/org/jboss/test/ws/jaxws/jbws2150/JBWS2150TestCase.java
Outdated
Show resolved
Hide resolved
|
@jimma updated according to the feedback, please review again. :) |
| @@ -78,7 +77,6 @@ public static WebArchive createDeployment() { | |||
| return archive; | |||
| } | |||
|
|
|||
| @Before | |||
| public void startContainerAndDeploy() throws Exception { | |||
There was a problem hiding this comment.
add the same TODO as in the other test?
| { | ||
| fail("Expected " + Arrays.asList(exp) + ", but was: null"); | ||
| } | ||
| protected static void assertEquals(String message, String expected, String actual) { |
There was a problem hiding this comment.
I find these method wrappers very misleading. I know it could possibly mean to rewrite more places but now we use methods from two classes (JBossWSTest and Assertions) with same name but different parameters - would it be possible to go without those wrapper methods and just keep one (JUnit5) style in whole project?
protected static void assertTrue(String reason, boolean condition)
protected static void assertFalse(String reason, boolean condition)
protected static void assertEquals(String message, char expected, char actual)
protected static void assertEquals(String message, String expected, String actual) <-- especially this one is really ambiguous
protected static void assertEquals(String message, Object expected, Object actual)
...etc
There was a problem hiding this comment.
thanks for the comment, I am good with removing these method wrappers. Yes, these were done for convenient to avoid rewriting assert methods in many places, but I get your point of misleading, will update it to use fully junit5 style only.
| try { | ||
| cjpMethod = ctx.getRequiredTestClass().getDeclaredMethod("getClientJarPaths"); | ||
| cjpMethod.setAccessible(true); | ||
| } catch (NoSuchMethodException nme) { |
There was a problem hiding this comment.
TestResultExtension is set for JBossWSTest and all descendants (which all will inherit getClientJarPaths() method) - so can this case ever happen? And even if it can, do we want to ignore it?
Also instead of using changing visibility via reflection, since we are only in test context, can't we simply make getClientJarPaths() method public to ease our lives?
There was a problem hiding this comment.
thanks. Right, makes the getClientJarPaths() public and not catching the NoSuchMethodException would be better here.
| @RunWith(Arquillian.class) | ||
| @ExtendWith(ArquillianExtension.class) | ||
| //https://issues.jboss.org/browse/JBEAP-5200 | ||
| @DisabledIfSystemProperties({@DisabledIfSystemProperty(named = "java.vendor", matches = "IBM Corporation"), |
There was a problem hiding this comment.
I propose to simple remove this, we are running JDK11+ anyway so this rule is obsolete
There was a problem hiding this comment.
indeed, we are running JDK11+, will remove this line.
There was a problem hiding this comment.
I think we'd better keep this. As we don't run IBM JDK 11 , this can probably gives use some hints that this isn't used to work with IBM JDK8, probably this still doesn't work with IBM JDK11 either.
| @@ -74,15 +72,13 @@ public static WebArchive createDeployments() { | |||
| return archive; | |||
| } | |||
|
|
|||
| @Before | |||
There was a problem hiding this comment.
again add TODO about arquiliian issue?
There was a problem hiding this comment.
OK, will check it thoroughly. 👍
… JBWS2150TestCase by the workaround for ARQ-2231
|
Updated according to the feedback. |
Issue: https://issues.redhat.com/browse/JBWS-4412
That this PRs try to do are:
junit:junittoorg.junit.jupiter:junit-jupiter, update arquillian.version to1.8.0.Final, update arquillian-wildfly-container.version to5.1.0.Beta1@EnableOnJDK, replace it with@EnabledOnJre@IgnoreContainer, not used in the project@IgnoreEnv, replace it with@DisabledIfSystemProperty@IgnoreJdk, replace it with@DisabledIfSystemPropertiesTestRule watcherin JBossWSTest to a static class callsed TestResultExtension, it will tries to callgetClientJarPathsmethod defined by the test class using reflection.DeployerorContainerControllerfrom@BeforeEachand@AfterEachmethods to each@Testmethod because there is an issue in arquillian 1.8.0.Final which fails to deploy the deployments in the life cycle methods. Note that this is temporary workaround, it is highly recommended to switch back when the issue gets fixed.