refactor (jkube-kit-generator-webapp): Migrate all tests from JUnit4 to JUnit5 (#1563) #1691
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Eclipse JKube CI ReportStarted new GH workflow run for #1691 (2022-10-03T13:01:17Z) ⚙️ JKube E2E Tests (3174210213)
|
Codecov Report
@@ Coverage Diff @@
## master #1691 +/- ##
============================================
- Coverage 52.95% 52.92% -0.03%
+ Complexity 3937 3936 -1
============================================
Files 464 464
Lines 20745 20752 +7
Branches 2809 2817 +8
============================================
- Hits 10985 10983 -2
- Misses 8637 8646 +9
Partials 1123 1123
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
manusa
requested changes
Aug 4, 2022
...ator/webapp/src/test/java/org/eclipse/jkube/generator/webapp/AppServerAutoDetectionTest.java
Outdated
Show resolved
Hide resolved
...t/generator/webapp/src/test/java/org/eclipse/jkube/generator/webapp/WebAppGeneratorTest.java
Outdated
Show resolved
Hide resolved
...ebapp/src/test/java/org/eclipse/jkube/generator/webapp/handler/JettyAppSeverHandlerTest.java
Outdated
Show resolved
Hide resolved
...ebapp/src/test/java/org/eclipse/jkube/generator/webapp/handler/JettyAppSeverHandlerTest.java
Outdated
Show resolved
Hide resolved
...ebapp/src/test/java/org/eclipse/jkube/generator/webapp/handler/JettyAppSeverHandlerTest.java
Outdated
Show resolved
Hide resolved
...bapp/src/test/java/org/eclipse/jkube/generator/webapp/handler/TomcatAppSeverHandlerTest.java
Outdated
Show resolved
Hide resolved
...app/src/test/java/org/eclipse/jkube/generator/webapp/handler/WildFlyAppSeverHandlerTest.java
Outdated
Show resolved
Hide resolved
...app/src/test/java/org/eclipse/jkube/generator/webapp/handler/WildFlyAppSeverHandlerTest.java
Outdated
Show resolved
Hide resolved
...app/src/test/java/org/eclipse/jkube/generator/webapp/handler/WildFlyAppSeverHandlerTest.java
Outdated
Show resolved
Hide resolved
...app/src/test/java/org/eclipse/jkube/generator/webapp/handler/WildFlyAppSeverHandlerTest.java
Outdated
Show resolved
Hide resolved
manusa
reviewed
Aug 4, 2022
...ator/webapp/src/test/java/org/eclipse/jkube/generator/webapp/AppServerAutoDetectionTest.java
Outdated
Show resolved
Hide resolved
manusa
requested changes
Aug 4, 2022
manusa
requested changes
Aug 4, 2022
| assertTrue(new File(appDir, "META-INF/").mkdirs()); | ||
| assertTrue(new File(appDir, "WEB-INF/").mkdirs()); | ||
| assertTrue(new File(appDir, descriptor).createNewFile()); | ||
| Path webInf = Files.createDirectories(folder.resolve("webapp" + i).resolve(appDir)); |
There was a problem hiding this comment.
This path is not resolving to webapp$i/META-INF/WEB-INF
Considering L139(L138), we need to keep the other appDir variable too.
There was a problem hiding this comment.
On (master branch) debugging this, I found out that on each iteration it is creating a directory webapp$i and inside it creates two different directories META-INF and WEB-INF, not nested as you said webapp$i/META-INF/WEB-INF
After resolving these different directories it resolves descriptor accordingly.
If I do like #1691 (comment) then it gives
java.nio.file.NoSuchFileException: /tmp/junit3247740821297201882/webapp0/META-INF/WEB-INF/WEB-INF/jboss-deployment-structure.xmlbecause jboss-deployment-structure.xml file needs to be resolved in webapp0/WEB-INF directory instead of webapp0/META-INF/WEB-INF/WEB-INF directory (nested WEB-INF)
and the path resolved in webapp$i/META-INF/WEB-INF after that, it resolved the $descriptor which contains an absolute path to the file, resulting in nested directories. And that causes the exception.
but on doing like
String appDir = descriptor.split("/")[0];
String file = descriptor.split("/")[1];
Path webInf = Files.createDirectories(folder.resolve("webapp" + i).resolve(appDir));
Files.createFile(webInf.resolve(file));it works because here we create a directory webapp$i and inside it a $appDir directory and inside $appDir a $file file, the $appDir and $file we get from the $descriptor because it contains the absolute path to the $file
There was a problem hiding this comment.
The #1691 (comment) code suggestion is wrong.
For some reason I assumed the directories were nested (probably too many iterations over the same issue).
The test should be migrated to keep the previous behavior but using the nio Files utils instead.
| final File artifactFile = new File(buildDirectory, "artifact.war"); | ||
| assertTrue(artifactFile.createNewFile()); | ||
| assertThat(artifactFile.createNewFile()).isTrue(); |
There was a problem hiding this comment.
Please use nio Files. I'm not sure if you're having trouble with this, or I'm not explaining correctly why we should do so.
There was a problem hiding this comment.
Sorry I missed this 😔
Let me be clear should I do something like this?
Files.createFile(buildDirectory.toPath().resolve("artifact.war"));| final File artifactFile = new File(buildDirectory, "artifact.war"); | ||
| assertTrue(artifactFile.createNewFile()); | ||
| assertThat(artifactFile.createNewFile()).isTrue(); |
| final File artifactFile = new File(buildDirectory, "artifact.war"); | ||
| assertTrue(artifactFile.createNewFile()); | ||
| assertThat(artifactFile.createNewFile()).isTrue(); |
...ebapp/src/test/java/org/eclipse/jkube/generator/webapp/handler/JettyAppSeverHandlerTest.java
Outdated
Show resolved
Hide resolved
...bapp/src/test/java/org/eclipse/jkube/generator/webapp/handler/TomcatAppSeverHandlerTest.java
Outdated
Show resolved
Hide resolved
...app/src/test/java/org/eclipse/jkube/generator/webapp/handler/WildFlyAppSeverHandlerTest.java
Outdated
Show resolved
Hide resolved
...app/src/test/java/org/eclipse/jkube/generator/webapp/handler/WildFlyAppSeverHandlerTest.java
Outdated
Show resolved
Hide resolved
...app/src/test/java/org/eclipse/jkube/generator/webapp/handler/WildFlyAppSeverHandlerTest.java
Outdated
Show resolved
Hide resolved
...app/src/test/java/org/eclipse/jkube/generator/webapp/handler/WildFlyAppSeverHandlerTest.java
Outdated
Show resolved
Hide resolved
manusa
requested changes
Aug 9, 2022
anurag-rajawat
force-pushed
the
webapp
branch
2 times, most recently
from
4cca83d
to
a218b70
Compare
anurag-rajawat
force-pushed
the
webapp
branch
2 times, most recently
from
47296d6
to
5666814
Compare
|
@manusa please review this PR |
anurag-rajawat
force-pushed
the
webapp
branch
from
5666814
to
fa82a3f
Compare
anurag-rajawat
force-pushed
the
webapp
branch
from
fa82a3f
to
c940c27
Compare
rohanKanojia
approved these changes
Sep 6, 2022
anurag-rajawat
force-pushed
the
webapp
branch
from
c940c27
to
34503d1
Compare
anurag-rajawat
force-pushed
the
webapp
branch
2 times, most recently
from
7f53543
to
414e13e
Compare
anurag-rajawat
force-pushed
the
webapp
branch
2 times, most recently
from
250c16e
to
90ec1f2
Compare
|
@rohanKanojia when I rename Also |
|
Umm, I tried renaming it using IntelliJ, seems to work fine for me: |
|
Now in changed files on github, it is deleted and a new one created, why? |
|
I'm sorry I'm not able to understand. I don't see any deleted file in eaacf11 |
|
I think there is some problem in GitHub interface. If you see your commit via |
|
Yes, it displayed the correct changes. |
sunix
reviewed
Sep 19, 2022
| @@ -121,28 +122,28 @@ public void testDefaultServer() throws IOException { | |||
| .build()).build(); | |||
|
|
|||
| AppServerHandler appServerHandler = new AppServerDetector(generatorContext).detect(null); | |||
| assertEquals("tomcat", appServerHandler.getName()); | |||
| assertThat(appServerHandler.getName()).isEqualTo("tomcat"); | |||
| } | |||
|
|
|||
| private void assertAppServerDescriptorApplicability(Object[] descriptors) throws IOException { | |||
There was a problem hiding this comment.
Maybe this method needs some more explanation (in a comment?). I read it several times but still don't know what it is doing.
There was a problem hiding this comment.
Here we are checking that if the app server is null or not specified, we should get the default server, which is tomcat.
Maybe this method name will explain what it is doing.
detect_withNullServer_shouldReturnTomcatAsDefaultServer()There was a problem hiding this comment.
I was refering to assertAppServerDescriptorApplicability
There was a problem hiding this comment.
OK I understand now ... what is confusing is that we are building the tuples (parameter/expectation) that are then extracted to be asserted ... Is it possible to make it simpler to implement and read.
There was a problem hiding this comment.
I tried to make it simple like this
private void assertAppServerDescriptorApplicability(String appDir, String file, boolean expected) throws IOException {
Random random = new Random();
int i = random.nextInt(1000);
Path webInf = Files.createDirectories(folder.resolve("webapp" + i).resolve(appDir));
Files.createFile(webInf.resolve(file));
GeneratorContext generatorContext = GeneratorContext.builder().project(JavaProject.builder()
.buildDirectory(webInf.toFile())
.plugins(Collections.emptyList())
.build()).build();
boolean result = new AppServerDetector(generatorContext).detect(null).isApplicable();
assertThat(result).isEqualTo(expected);
}but due to this statement
https://github.com/eclipse/jkube/blob/40ce95a6366986bd005b51420a799f09b5e075f8/jkube-kit/generator/webapp/src/test/java/org/eclipse/jkube/generator/webapp/AppServerAutoDetectionTest.java#L132
we need a random integer or we can change signature of assertAppServerDescriptorApplicability method to accept an additional argument.
There was a problem hiding this comment.
We can simplify assertAppServerDescriptorApplicability method like this
private void assertAppServerDescriptorApplicability(String appDir, String file, boolean expected) throws IOException {
File webInf = Files.createDirectories(folder.resolve("webapp").resolve(appDir)).toFile();
Files.createFile(webInf.toPath().resolve(file));
try {
GeneratorContext generatorContext = GeneratorContext.builder().project(JavaProject.builder()
.buildDirectory(webInf)
.plugins(Collections.emptyList())
.build()).build();
boolean result = new AppServerDetector(generatorContext).detect(null).isApplicable();
assertThat(result).isEqualTo(expected);
} finally {
FileUtils.deleteDirectory(webInf);
}
}will this work?
There was a problem hiding this comment.
then, If it has to be in another PR it works for me. If this refactor is not related to the migration from JUnit4 to JUnit5 it should be in another PR (I was mentioning that here in case you could use a JUnit5 feature to make that clearer)
There was a problem hiding this comment.
Sure, I see #1662 that's why I've not refactored it in this PR.
anurag-rajawat
force-pushed
the
webapp
branch
from
eaacf11
to
00d3d2b
Compare
…to JUnit5 (eclipse-jkube#1563) Signed-off-by: Anurag Rajawat <anuragsinghrajawat22@gmail.com>
Signed-off-by: Anurag Rajawat <anuragsinghrajawat22@gmail.com>
anurag-rajawat
force-pushed
the
webapp
branch
from
00d3d2b
to
a757ba4
Compare
|
@manusa please review this PR |
There are still some renamed variables that are not OK in this PR, I'll send a PR towards your branch to see if I can explain this better. |
sunix
approved these changes
Oct 3, 2022
Signed-off-by: Marc Nuri <marc@marcnuri.com>
I created anurag-rajawat#2 with some suggestions |
review: refactor: AppServerAutoDetectionTest uses parameters
@manusa thank you so much! |
manusa
approved these changes
Oct 3, 2022
|
Kudos, SonarCloud Quality Gate passed!
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #1563
Type of change
test, version modification, documentation, etc.)
Checklist