[SHRINKRES-342] Migrate to JUnit5 #241
Conversation
| import org.junit.Before; | ||
| import org.junit.BeforeClass; | ||
| import org.junit.Test; | ||
| import org.junit.jupiter.api.*; |
There was a problem hiding this comment.
I have missed that. All the occurrences should be fixed.
xjusko
force-pushed
the
migrate-to-junit5
branch
from
52fb084
to
ce0b3c3
Compare
| @@ -1,19 +1,3 @@ | |||
| /* | |||
There was a problem hiding this comment.
This was not intentional, probably some random input from my keyboard... Thanks for noticing it.
| System.setProperty(MavenSettingsBuilder.ALT_MAVEN_OFFLINE, "true"); | ||
| @Test | ||
| void overrideOfflineFlag() { | ||
| System.setProperty(MavenSettingsBuilder.ALT_MAVEN_OFFLINE, "true"); |
There was a problem hiding this comment.
The problem I see in this change is that in case of a test failure the property is still set and might affect other tests either in this testsuite or somewhere else (as it's a system property). I'd propose to change this testsuite in this PR and create a proper cleanup method AfterEach
| Assertions.assertThrows(NoResolvedResultException.class, () -> { | ||
| Maven.configureResolver().fromFile("target/settings/profiles/settings-jetty.xml") | ||
| .resolve("junit:junit:3.8.2").withTransitivity().as(File.class); | ||
| Assertions.fail("Artifact junit:junit:3.8.2 should not be present in local repository"); |
There was a problem hiding this comment.
Isn't this Assertions.fail redundant?
pom.xml
Outdated
| <dependency> | ||
| <groupId>org.hamcrest</groupId> | ||
| <artifactId>hamcrest</artifactId> | ||
| <version>2.2</version> |
There was a problem hiding this comment.
I believe Hamcrest released a new version 3.0, can that be used?
|
Good job @xjusko , just a couple of things I noticed and one rebase and I believe we can merge this |
xjusko
force-pushed
the
migrate-to-junit5
branch
2 times, most recently
from
a72907e
to
d619cb6
Compare
xjusko
force-pushed
the
migrate-to-junit5
branch
from
d619cb6
to
9fc0cf3
Compare
https://issues.redhat.com/browse/SHRINKRES-342