test cover `DefaultModelProcessor#read` #2304

Pankraz76 · 2025-05-08T06:42:05Z

Pull #2304: Modernize codebase with Java improvements - test DefaultModelProcessor#read

enable for:

Pull #2292: Modernize codebase with Java improvements - SOC DefaultModelProcessor#read #2292
Pull #2291: Modernize codebase with Java improvements - Unnecessary throws #2291

100 as everything else is enough for a tremendous project like maven:

…faultModelProcessor#read

Pankraz76 · 2025-05-08T07:52:45Z

this one important to give liberty on code back to us; making code liquide while logic remains tight and dusty.

Pankraz76 · 2025-05-08T07:55:43Z

ideally when merging this, the test will be obsolet in follow up, allowing any adjustment:

Pull #2292: Modernize codebase with Java improvements - SOC DefaultModelProcessor#read #2292

then we have awareness that coverage is a real deal and not a nice gimmick.

Kindly request your feedback @gnodet @elharo.

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java

Pankraz76 · 2025-05-08T08:00:50Z

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java

+        return project;
    }

-    private Model doRead(XmlReaderRequest request) throws IOException {


This is wrong/counterfeit because:

Currently, the API is designed to throw XmlReaderException. Check the implemented interface, there's no IOException thrown here.

#2292 (comment)

impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelProcessorTest.java

elharo · 2025-05-08T10:52:23Z

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java

-            } catch (IOException e) {
-                exceptions.forEach(e::addSuppressed);
-                throw e;
+            } catch (RuntimeException ex) {


Per Effective Java don't catch a raw runtime exception, only the specific subclass you expect to be thrown

manage to change to } catch (ModelParserException e) {

as IOError is a sitting duck ready, for refactoring into void:

#2292 (comment)

It's not thrown, that's right. So do not add a throw clause.

…faultModelProcessor#read

Pankraz76 · 2025-05-08T12:56:50Z

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java

+                        return parent.get().withPomFile(pomFile);
                    }
                } catch (ModelParserException e) {
                    exceptions.add(e);


is it mandatory to continue if an error occurs to give full error report? Or can we have here early exit if the branch kicks in like above in its counterpart?

I suppose it depends on whether one parser throwing an exception invalidates answers from subsequent parsers in the loop. I suspect it doesn't since you skip subsequent parsers once one returns.

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java

elharo · 2025-05-08T15:13:51Z

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java

+                        return parent.get().withPomFile(pomFile);
                    }
                } catch (ModelParserException e) {
                    exceptions.add(e);


I suppose it depends on whether one parser throwing an exception invalidates answers from subsequent parsers in the loop. I suspect it doesn't since you skip subsequent parsers once one returns.

elharo · 2025-05-08T15:15:26Z

impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelProcessorTest.java

+        void readWithParserThrowingExceptionShouldCollectException() {
+            ModelXmlFactory factory = mock(ModelXmlFactory.class);
+            ModelParser parser = mock(ModelParser.class);
+            XmlReaderRequest request = mock(XmlReaderRequest.class);


does this have to be mocked? can you just use the class?

elharo · 2025-05-08T15:16:32Z

impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelProcessorTest.java

+        when(mockSource.getPath()).thenReturn(Path.of("other/pom.xml"));
+        ModelParser parser = mock(ModelParser.class);
+        when(parser.locate(any())).thenReturn(Optional.of(mockSource));
+        assertTrue(assertThrows(IllegalArgumentException.class, () -> new DefaultModelProcessor(


nesting asserts is strange. Maybe don't do this, or split it into two separate statements

elharo · 2025-05-08T15:17:34Z

impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelProcessorTest.java

+    }
+
+    @Nested
+    class WithTmpFiles {


This is strange. I haven't seen nested test classes before. Might be fine, but is there a strong reason for this?

SOC is kind of crazy at first, but it can be handy—and necessary for special test patterns. It simplifies complex scenarios without forcing to massively copy or abstract test code, as logic can now be shared and layered in one place.

It’s similar to a dedicated class but with inheritance, allowing special sub-conditions. That said, it’s flexible and therefore has significant trade-offs (both good and bad side effects).

JUnit Nested Classes: Quick Pros & Cons

Pros:
✅ Better organization
✅ Clearer test grouping
✅ Flexible setup per group
✅ Reduces duplication
✅ Good for stateful testing

Cons:
❌ Adds complexity
❌ IDE navigation may suffer
❌ Risk of over-nesting
❌ Must be non-static
❌ Learning curve for new devs

Best for: Complex tests with shared setups or multiple states.

elharo · 2025-05-08T15:18:18Z

impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelProcessorTest.java

+
+        @Test
+        void locateExistingPomWithDirectoryContainingPom() throws IOException {
+            testProjectDir = createTempDirectory(tempDir, "project");


This test might belong elsewhere if it's not using the same fixture.

elharo · 2025-05-08T15:18:23Z

impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelProcessorTest.java

+
+        @Test
+        void locateExistingPomWithDirectoryWithoutPom() throws IOException {
+            testProjectDir = createTempDirectory(tempDir, "project");


This test might belong elsewhere if it's not using the same fixture.

elharo · 2025-05-08T15:18:28Z

impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelProcessorTest.java

+
+        @Test
+        void locateExistingPomWithPomFile() throws IOException {
+            testPomFile = Files.createTempFile(tempDir, "pom", ".xml");


This test might belong elsewhere if it's not using the same fixture.

gnodet · 2025-05-08T18:24:33Z

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java

-        } else {
-            return null;
-        }
+    private static Optional<Model> parent(XmlReaderRequest request, ModelParser parser, Path projectDirectory) {


A parent usually refers to the parent model, so I'd rather use parentDirectory to make it clear what we're looking for.

gnodet · 2025-05-08T18:26:12Z

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java


-    private Model doRead(XmlReaderRequest request) throws IOException {
-        return modelXmlFactory.read(request);
+    private static void throwErrorsSuppressed(List<ModelParserException> exceptions) throws IOException {


We're not using the IOException in the API, so not sure why we'd want to build one here. Model parsing exception aren't necessarily IoExceptions.

…faultModelProcessor#read

Pankraz76 · 2025-05-09T10:18:31Z

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java

-                        .orElse(null))
-                .filter(Objects::nonNull)
-                .findFirst()
-                .orElseGet(() -> doLocateExistingPom(projectDirectory));


unpleasant do prefix is fix for naming collision @elharo

Pankraz76

finally covered, small polish.

Pankraz76 · 2025-05-09T10:19:15Z

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java

+                        .orElseGet(() -> locateExistingPomWithFallback(projectDirectory)));
+    }
+
+    private static Path throwIfWrongProjectDirLocation(Path projectDirectory, Path pom) {


Pankraz76 · 2025-05-09T10:19:22Z

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java

        return pom;
    }

+    private Path locateExistingPomWithFallback(Path project) {


impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java

Pankraz76 · 2025-05-09T10:20:01Z

impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelProcessorTest.java

+
+    @Test
+    void locateExistingPomShouldHandleRegularFileInput() throws IOException {
+        // Create a temporary file


Pankraz76 · 2025-05-13T18:22:05Z

wip reopen

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 8, 2025

Pull apache#2304: test DefaultModelProcessor#read

0482492

Pankraz76 force-pushed the functional-DefaultModelProcessor-test branch from 25dbfc7 to 0482492 Compare May 8, 2025 06:43

Pankraz76 changed the title ~~Pull #2292: Modernize codebase with Java improvements - test DefaultModelProcessor#read~~ Modernize codebase with Java improvements - test DefaultModelProcessor#read May 8, 2025

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 8, 2025

Pull apache#2304: Modernize codebase with Java improvements - test De…

542101a

…faultModelProcessor#read

Pankraz76 force-pushed the functional-DefaultModelProcessor-test branch from 0482492 to 542101a Compare May 8, 2025 06:43

Pankraz76 mentioned this pull request May 8, 2025

Pull #2292: Modernize codebase with Java improvements - SOC DefaultModelProcessor#read #2292

Closed

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 8, 2025

Pull apache#2304: Modernize codebase with Java improvements - test De…

7ed0ace

…faultModelProcessor#read

Pankraz76 force-pushed the functional-DefaultModelProcessor-test branch from 542101a to 7ed0ace Compare May 8, 2025 07:18

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 8, 2025

Pull apache#2304: Modernize codebase with Java improvements - test De…

e46da18

…faultModelProcessor#read

Pankraz76 force-pushed the functional-DefaultModelProcessor-test branch from 7ed0ace to e46da18 Compare May 8, 2025 07:33

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 8, 2025

Pull apache#2304: Modernize codebase with Java improvements - test De…

02b5deb

…faultModelProcessor#read

Pankraz76 force-pushed the functional-DefaultModelProcessor-test branch from e46da18 to 02b5deb Compare May 8, 2025 07:42

Pankraz76 marked this pull request as ready for review May 8, 2025 07:48

Pankraz76 commented May 8, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java Show resolved Hide resolved

Pankraz76 commented May 8, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java Show resolved Hide resolved

Pankraz76 commented May 8, 2025

View reviewed changes

elharo requested changes May 8, 2025

View reviewed changes

Pankraz76 changed the title ~~Modernize codebase with Java improvements - test DefaultModelProcessor#read~~ Modernize codebase with Java improvements - test DefaultModelProcessor#read May 8, 2025

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 8, 2025

Pull apache#2304: Modernize codebase with Java improvements - test De…

b6809cd

…faultModelProcessor#read

Pankraz76 force-pushed the functional-DefaultModelProcessor-test branch from 02b5deb to b6809cd Compare May 8, 2025 11:49

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 8, 2025

Pull apache#2304: test DefaultModelProcessor#read

0b043ae

Pankraz76 force-pushed the functional-DefaultModelProcessor-test branch from b6809cd to 0b043ae Compare May 8, 2025 12:55

Pankraz76 commented May 8, 2025

View reviewed changes

Pankraz76 requested a review from elharo May 8, 2025 13:00

Pankraz76 pushed a commit to Pankraz76/maven that referenced this pull request May 8, 2025

Pull apache#2304: test DefaultModelProcessor#read

92b5e79

Pankraz76 force-pushed the functional-DefaultModelProcessor-test branch from 0b043ae to 92b5e79 Compare May 8, 2025 13:03

elharo reviewed May 8, 2025

View reviewed changes

Pankraz76 mentioned this pull request May 8, 2025

Fix unused stream in DefaultPluginXmlFactory#write #2303

Closed

gnodet requested changes May 8, 2025

View reviewed changes

Pull apache#2304: Modernize codebase with Java improvements - test De…

49d13d9

…faultModelProcessor#read

Pankraz76 force-pushed the functional-DefaultModelProcessor-test branch from 92b5e79 to 49d13d9 Compare May 9, 2025 10:17

Pankraz76 commented May 9, 2025

View reviewed changes

Pankraz76 changed the title ~~Modernize codebase with Java improvements - test DefaultModelProcessor#read~~ test cover DefaultModelProcessor#read May 9, 2025

Pankraz76 marked this pull request as draft May 10, 2025 23:03

Pankraz76 closed this May 13, 2025

Uh oh!

test cover DefaultModelProcessor#read #2304

test cover DefaultModelProcessor#read #2304

Uh oh!

Conversation

Pankraz76 commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pankraz76 commented May 8, 2025

Uh oh!

Pankraz76 commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

JUnit Nested Classes: Quick Pros & Cons

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 commented May 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

test cover `DefaultModelProcessor#read` #2304

test cover `DefaultModelProcessor#read` #2304

Pankraz76 commented May 8, 2025 •

edited

Loading

Pankraz76 commented May 8, 2025 •

edited

Loading

Pankraz76 May 8, 2025 •

edited

Loading

Pankraz76 May 9, 2025 •

edited

Loading