test: improve ProjectModelResolver tests - add wrong range format cases and clean up throws declarations

[TheRealHaui]

Improve test coverage for ProjectModelResolver:

• Add testResolveParentThrowsUnresolvableModelExceptionWhenUsingWrongRangeFormat: verifies that an invalid version range format (e.g. [1:2)) is rejected with a clear error message for parent resolution
• Add testResolveDependencyThrowsUnresolvableModelExceptionWhenUsingWrongRangeFormat: same coverage for dependency resolution
• Remove unnecessary throws Exception declarations from 6 existing test methods that use assertThrows (no checked exceptions escape)

[cstamas]

LGTM. @gnodet ?

[gnodet]

Review: Added additional tests to ProjectModelResolverTest

Good addition — these tests exercise a meaningful error path: what happens when a user provides a syntactically invalid version range (: instead of , as separator). This covers the catch (VersionRangeResolutionException e) branch in ProjectModelResolver.resolveModel() that was previously untested. The throws Exception cleanup is also a nice touch.

The tests follow the existing conventions in the file (JUnit 5 Assertions, consistent naming, matching parent/dependency test pairs). @cstamas has already given an LGTM.

Observations

Low — Full message assertions depend on Aether library text

The assertions use assertEquals on the full exception message string:

"Failed to resolve version range for org.apache:apache:pom:[1:2): Invalid version range [1:2), single version must be surrounded by []"

This entire message originates from the Aether VersionRangeResolutionException — Maven just passes it through via e.getMessage(). If the resolver library changes this wording, the test breaks for reasons unrelated to Maven.

The existing tests in this file that use assertEquals assert on messages that Maven itself constructs (the String.format(...) calls at lines 197-198 and 209-210 of ProjectModelResolver.java). The one existing test that checks a message from an external library (testResolve*WhenNotFound) uses assertTrue(contains(...)) instead of assertEquals.

For consistency and resilience, consider assertTrue(e.getMessage().contains("Invalid version range")) or assertThat(e.getMessage()).contains("Invalid version range").

This is a minor observation — the tests are correct and useful as-is.

---

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

[gnodet]

Clean test-only PR that adds two new test cases covering the malformed version range error path in ProjectModelResolver and removes unnecessary throws Exception declarations from six existing test methods. All changes are correct and follow the file's existing conventions.

One minor observation: the assertEquals assertions check the full exception message string from VersionRangeResolutionException. If Aether changes this wording, these tests would break. Consider using assertTrue(e.getMessage().contains("Invalid version range")) for resilience — though the tests are correct as-is. (Already noted by a prior reviewer.)

CI is fully green (the single macOS+JDK17 IT failure is unrelated).

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[slachiewicz]

The last CI run on this branch was 2025-11-28, and GitHub has deleted the logs since, so there's no way to tell whether the change still builds against master.

A rebase triggers a fresh run and makes the state readable again. If you'd rather not carry it forward, closing is equally useful — the PR isn't being ignored, it's unreadable.

This comment was created with AI assistance.

[TheRealHaui]

@slachiewicz,
I think the only thing necessary to do here is to re-start the CI builds.

[slachiewicz]

I can't trigger it as it's old run and it's possible only for 30 days since last run

[gnodet-bot]

Re-review: Added additional tests to ProjectModelResolverTest

Verdict: 💬 COMMENT

New commit (749b45f1) is a merge of origin/master — no changes to the test additions themselves. The prior code review findings remain as-is.

Unaddressed nit from prior reviews (raised twice)

Both prior reviews noted that the new assertEquals assertions check the full exception message from VersionRangeResolutionException:

assertEquals(
    "Failed to resolve version range for org.apache:apache:pom:[1:2): Invalid version range [1:2), single version must be surrounded by []",
    e.getMessage());

This message originates entirely from the Aether resolver library — Maven only forwards it. If the resolver ever changes its wording, this test breaks for reasons unrelated to Maven.

The existing test for the "not found" error path (testResolveParentThrowsUnresolvableModelExceptionWhenNotFound) uses assertTrue(contains(...)) for messages from external libraries. The new tests should follow the same convention:

assertTrue(e.getMessage().contains("Invalid version range"), e.getMessage());

@gnodet has already approved this PR (APPROVE from the human account). The CI is green. The only remaining item is this robustness nit.

---

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[github-actions]

⚠️ License declaration missing

The pull request description must contain at least one of the following lines with a checked box ([x]):

- [ ] I hereby declare this contribution to be licensed under the [Apache License Version 2.0, January 2004](https://www.apache.org/licenses/LICENSE-2.0)
- [ ] I hereby declare that I have filed an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).

This comment will be hidden automatically once the description is fixed.