gnodet commented on PR #1904: URL: https://github.com/apache/maven-resolver/pull/1904#issuecomment-4647273566
One additional thought on **test coverage**: the existing `ConflictResolverTest` covers several cycle scenarios (`winnerCycleRemoved`, `subtreeRepeatedSameInstanceWithRealCycle`, `nettyBoringSslExample`), but none of them reproduce the specific topology that triggered the NPE — where a cycle-flagged node has children that are *not* in the cycle, and those children's subtrees contain unique conflictIds not reachable by any other path. A test like the following would nail down the exact regression from 5e97fd15 and prevent it from recurring: ```java // dom4j-like topology: // root -> A -> B (in cycle with A via cyclicPredecessors) -> C (unique, only reachable through B) // -> A' (same conflictId as A, actual cycle) // Old code: B flagged as cycle → C never explored → partitions.get(C_id) == null → NPE // New code: B not a cycle (B_id ∉ [root_id, A_id]) → C explored normally → cycle broken at A' ``` Not blocking the merge, but would be a nice safety net. The existing tests exercise the *correction* path (cycle properly broken) but not the *non-regression* path (non-cycle siblings properly explored). — gnodet -- 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]
