gnodet-bot commented on code in PR #13881:
URL: https://github.com/apache/maven/pull/13881#discussion_r4502203785
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultSettingsParserTest.java:
##########
@@ -509,17 +506,23 @@ void unnamedParserWorksInNativeContainer() throws
Exception {
}
@Test
- void unnamedParserConflictIsReported() throws Exception {
- var parsers = new HashMap<String, SettingsParser>();
- parsers.put(null, new PropertiesSettingsParser());
- parsers.put("properties", new PropertiesSettingsParser());
- var error =
- assertThrows(SettingsBuilderException.class, () ->
build(source("settings.properties", ""), parsers));
+ void parserConflictIsReported() throws Exception {
+ var parser1 = mock(SettingsParser.class);
+ when(parser1.id()).thenReturn("parser1");
+ var parser2 = mock(SettingsParser.class);
+ when(parser2.id()).thenReturn("parser2");
Review Comment:
๐งน **Stale mock stubs โ dead code**
`parser1.id()` and `parser2.id()` are stubbed here as `"parser1"` and
`"parser2"`, then immediately overwritten on lines 514 and 516 with
`"parser-a"` and `"parser-b"`. Mockito uses the **last** stub, so these two
`when()` calls are never effective.
```suggestion
var parser2 = mock(SettingsParser.class);
```
##########
its/core-it-suite/src/test/resources/mng-7836-alternative-pom-syntax/maven-hocon-extension/src/main/java/org/apache/maven/hocon/HoconModelReader.java:
##########
@@ -41,6 +41,10 @@
@Priority(1)
public class HoconModelReader implements ModelParser {
+ public String id() {
Review Comment:
๐งน **Missing `@Override`**
The new `id()` method implements `ModelParser.id()` from the SPI but lacks
`@Override`. All other implementations in this PR include it (e.g.
`XmlSettingsParser`, `DiTest` inner classes). Same issue applies to
`DumbModelParser1`โ`4` and the `PropertiesSettingsParser` IT fixture.
```suggestion
@Override
public String id() {
```
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultSettingsParserTest.java:
##########
@@ -265,35 +265,31 @@ void supportsFailurePreventsXmlFallback() throws
Exception {
assertSupportsFailureStopsSelectionAndParsing("broken", false);
}
- @Test
- void unnamedSupportsFailurePreventsXmlFallback() throws Exception {
- assertSupportsFailureStopsSelectionAndParsing(null, false);
- }
-
@Test
void supportsFailurePreventsPreviouslyMatchedParser() throws Exception {
assertSupportsFailureStopsSelectionAndParsing("broken", true);
}
- @Test
- void unnamedSupportsFailurePreventsPreviouslyMatchedParser() throws
Exception {
- assertSupportsFailureStopsSelectionAndParsing(null, true);
- }
-
- private void assertSupportsFailureStopsSelectionAndParsing(String
providerName, boolean matchingFirst)
+ private void assertSupportsFailureStopsSelectionAndParsing(String
parserId, boolean matchingFirst)
throws Exception {
SettingsParser matching = mock(SettingsParser.class);
+ when(matching.id()).thenReturn("matching");
SettingsParser broken = mock(SettingsParser.class);
+ when(broken.id()).thenReturn("broken");
SettingsParser unvisited = mock(SettingsParser.class);
+ when(unvisited.id()).thenReturn("unvisited");
+ when(matching.id()).thenReturn("matching");
when(matching.supports(any())).thenReturn(true);
+ when(broken.id()).thenReturn(parserId);
var failure = new IllegalStateException();
when(broken.supports(any())).thenThrow(failure);
- var parsers = new LinkedHashMap<String, SettingsParser>();
+ when(unvisited.id()).thenReturn("unvisited");
Review Comment:
๐งน **Redundant/overwritten mock stubs**
This block has three problems:
- `matching.id()` is stubbed to `"matching"` on line 276, then identically
again on line 281 โ exact duplicate.
- `broken.id()` is stubbed to `"broken"` on line 278, then immediately
overwritten with `parserId` on line 283 โ first stub is never used.
- `unvisited.id()` is stubbed to `"unvisited"` on line 280, then identically
again on line 286 โ exact duplicate.
Lines 276, 278, and 280 can be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
