chore: ArtifactHandlerTest improve tests on packaging#2375
chore: ArtifactHandlerTest improve tests on packaging#2375Pankraz76 wants to merge 1 commit intoapache:masterfrom
ArtifactHandlerTest improve tests on packaging#2375Conversation
Pankraz76
changed the title
@SuppressWarnings("checkstyle:UnusedLocalVariable")ArtifactHandlerTest add test assertion to resolve @SuppressWarnings("checkstyle:UnusedLocalVariable")
Pankraz76
changed the title
ArtifactHandlerTest add test assertion to resolve @SuppressWarnings("checkstyle:UnusedLocalVariable")ArtifactHandlerTest add assertion to resolve @SuppressWarnings("checkstyle:UnusedLocalVariable")
impl/maven-core/src/test/java/org/apache/maven/artifact/handler/ArtifactHandlerTest.java
Outdated
Show resolved
Hide resolved
Pankraz76
force-pushed
the
fix-ArtifactHandlerTest
branch
2 times, most recently
from
b60fd48 to
bc0ea91
Compare
Pankraz76
force-pushed
the
fix-ArtifactHandlerTest
branch
from
bc0ea91 to
44a5d95
Compare
impl/maven-core/src/test/java/org/apache/maven/artifact/handler/ArtifactHandlerTest.java
Outdated
Show resolved
Hide resolved
Pankraz76
force-pushed
the
fix-ArtifactHandlerTest
branch
from
44a5d95 to
582c71a
Compare
| assertEquals(handler.getExtension(), extension, type + " extension"); | ||
| // Packaging/Directory is Maven1 remnant!!! | ||
| // assertEquals(handler.getPackaging(), packaging, type + " packaging"); | ||
| assertThat(handler.getPackaging()).contains(packaging); |
There was a problem hiding this comment.
test-jar != jar
this could be investigated, if only this is a combination not matching and why so.
There was a problem hiding this comment.
could merge will check up.
There was a problem hiding this comment.
I don't understand why we're not just using assertEquals here.
There was a problem hiding this comment.
because test-jar != jar might be the only exception, but equals is the right but not working.
This could be an bug because the packaging is different. Will investigate.
There was a problem hiding this comment.
all hyphen needs contains. we can continue but i wont want to ask any more questions and would take increment.
Whats your call?
Pankraz76
force-pushed
the
fix-ArtifactHandlerTest
branch
from
582c71a to
7872cca
Compare
Pankraz76
force-pushed
the
fix-ArtifactHandlerTest
branch
from
7872cca to
f6c6200
Compare
| assertEquals("jar", packaging); | ||
|
|
||
| } else if (handler.getPackaging().equals("ejb-client")) { | ||
| assertEquals("ejb", packaging); |
There was a problem hiding this comment.
both look like bug to me. maven-archetype",
"maven-plugin", "java-source" are all equal so why not these two ? So its not a hyphen thing in general, right?
There was a problem hiding this comment.
I might be starting to see what's going on here. I think you're hitting a weird issue (though WAI) about file extension vs dependency type. See https://maven.apache.org/ref/3.9.9/maven-core/artifact-handlers.html and pages I can't put my finger on right now, but updated a few months ago.
That being said, I am more convinced now that this approach is incorrect. We want one single assertEquals that tests a specific value, not contains something from a collection and not "if this assert that". This might mean splitting into multiple test methods. DRY does not apply to tests and asserts. Unit tests should be very straight-forward with no branching and no conditionals.
There was a problem hiding this comment.
Pankraz76
force-pushed
the
fix-ArtifactHandlerTest
branch
from
f6c6200 to
6a90f9b
Compare
| assertEquals("jar", packaging); | ||
|
|
||
| } else if (handler.getPackaging().equals("ejb-client")) { | ||
| assertEquals("ejb", packaging); |
There was a problem hiding this comment.
I might be starting to see what's going on here. I think you're hitting a weird issue (though WAI) about file extension vs dependency type. See https://maven.apache.org/ref/3.9.9/maven-core/artifact-handlers.html and pages I can't put my finger on right now, but updated a few months ago.
That being said, I am more convinced now that this approach is incorrect. We want one single assertEquals that tests a specific value, not contains something from a collection and not "if this assert that". This might mean splitting into multiple test methods. DRY does not apply to tests and asserts. Unit tests should be very straight-forward with no branching and no conditionals.
Pankraz76
changed the title
ArtifactHandlerTest add assertion to resolve @SuppressWarnings("checkstyle:UnusedLocalVariable")ArtifactHandlerTest improve tests on packaging
Pankraz76
force-pushed
the
fix-ArtifactHandlerTest
branch
from
6a90f9b to
d67f637
Compare
| container.lookup(ArtifactHandlerManager.class).getArtifactHandler(type); | ||
| assertEquals(handler.getExtension(), extension, type + " extension"); | ||
| // Packaging/Directory is Maven1 remnant!!! | ||
| // assertEquals(handler.getPackaging(), packaging, type + " packaging"); |
There was a problem hiding this comment.
better some special workaround assert than no at all!
Agree upon test code allowed some flexibility like having commented out code. So lets use this liberty to improve the test by testing the production code.
This topic remains, but we have cleared up the doing and increased coverage.
If/as no ability to fix it yet, might consider take the increment and go for next increment.
3 participants
provide only fixup for UnusedLocalVariable, no refactoring like in
@SuppressWarnings("checkstyle:UnusedLocalVariable")#2365