test: Migrate ImportTest to JUnit5 #4225
Conversation
| @@ -202,7 +196,7 @@ public void testImportOfAnInnerClassInAClassPackage() throws Exception { | |||
| final CtMethod<?> methodVisit = client.getMethodsByName("visit").get(0); | |||
|
|
|||
| final CtType<Object> innerClass = factory.Type().get("spoon.test.imports.testclasses.DefaultClientClass$InnerClass"); | |||
| assertEquals("Type of the method must to be InnerClass accessed via DefaultClientClass.", innerClass, methodVisit.getType().getDeclaration()); | |||
| assertEquals(innerClass, methodVisit.getType().getDeclaration(), "Type of the method must to be InnerClass accessed via DefaultClientClass."); | |||
There was a problem hiding this comment.
There were lots of these cases where assertEquals(message, expected, actual) is now assertEquals(expected, actual, message). Pretty much all of them were fixed by simply permuting arguments.
| @@ -453,9 +447,9 @@ void checkCanAccess(String aClassName, boolean isInterface, boolean canAccessCli | |||
| CtTypeReference<?> accessType; | |||
|
|
|||
| if(canAccessClientClass) { | |||
| assertTrue("ClientClass should have access to "+aClassName+" but it has not", aClientClass.canAccess(target)); | |||
| assertTrue(aClientClass.canAccess(target), "ClientClass should have access to "+aClassName+" but it does not have access"); | |||
There was a problem hiding this comment.
Similarly, assertTrue(message, subject) and assertFals(message, subject) are now assertTrue(subject, message) and assertFalse(subject, message).
There were a couple of cases in here where I also clarified the language on the message a bit to be more idiomatically English.
| @@ -663,7 +657,7 @@ public void testDeepNestedStaticPathWithTypedParameter() { | |||
| fail(e.getMessage()); | |||
| } | |||
| CtClass<?> mm = launcher.getFactory().Class().get("spoon.test.imports.testclasses2.StaticWithNested"); | |||
| assertTrue("new spoon.test.imports.testclasses2.StaticWithNested.StaticNested.StaticNested2<K>();", mm.toString().contains("new spoon.test.imports.testclasses2.StaticWithNested.StaticNested.StaticNested2<K>();")); | |||
| assertThat(mm.toString(), containsString("new spoon.test.imports.testclasses2.StaticWithNested.StaticNested.StaticNested2<K>();")); | |||
There was a problem hiding this comment.
Then there were many of these cases: assertTrue(someString.contains(someOtherString) or assertFalse(!someString.contains(someOtherString)). I swapped these to assertThat(someString, containsString(someOtherString)) and assertThat(someString, CoreMatchers.not(containsString(someOtherString))), which yields a more-descriptive message when assertions fail.
| assertThat( | ||
| "The file should not contain a static import to the inner enum method values", | ||
| output, | ||
| CoreMatchers.not(containsString("import static spoon.test.imports.testclasses.StaticImportsFromEnum$DataElement.values;")) |
There was a problem hiding this comment.
To make the assertions more readable, could you use static imports instead? I.e. such that this looks like so instead:
| CoreMatchers.not(containsString("import static spoon.test.imports.testclasses.StaticImportsFromEnum$DataElement.values;")) | |
| not(containsString("import static spoon.test.imports.testclasses.StaticImportsFromEnum$DataElement.values;")) |
Instead of importing CoreMatchers, you do import static org.hamcrest.CoreMatchers.not. It's the most common way of using the matchers, and will match the style of other test classes. It's also IMO the most readable.
There was a problem hiding this comment.
Sure, can do! I went back and forth on that one, to be honest; not(...) is more readable than CoreMatchers.not(...), but I also know that it's a common-enough method name to be likely to have ambiguous namespace resolution when just reading the code without an IDE's help, so I wasn't sure which way to go with it.
I'll make the change here and puth that up.
There was a problem hiding this comment.
Change applied. Thanks for the review!
#3919
Another test class migration. This one had several cases where the JUnit
assert<Something>(...)signature changed, so I took the opportunity to swap toassertThat(...)using already-available Hamcrest core matchers, which should help shield against any future signature changes (since Hamcrest is a pretty stable assertions API, or at least a widely-used one).