test(model): Fix the test for cyclic dependencies#11426
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
sschuberth
force-pushed
the
deps-comp-imps
branch
from
40f7f35 to
509af51
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11426 +/- ##
=========================================
Coverage 58.40% 58.40%
+ Complexity 1759 1758 -1
=========================================
Files 355 355
Lines 13219 13219
Branches 1314 1314
=========================================
Hits 7721 7721
Misses 5008 5008
Partials 490 490
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| .build(checkReferences = false) | ||
|
|
||
| graph.nodes shouldHaveSize 6 | ||
| graph.edges shouldHaveSize 5 |
There was a problem hiding this comment.
It could be nice to verify the actual edges here to demonstrate how the cycles are broken.
There was a problem hiding this comment.
While debugging this with PNPM-specific changes on top, it looks to be as if indeed the cycle is broken at the "wrong" place. I need to further investigate this; moving back to draft.
There was a problem hiding this comment.
as if indeed the cycle is broken at the "wrong" place.
This was fixed by #11575. I've added verification of the actual edges to the test now.
| val depFoo = createDependency("org.foo", "foo", "1.2.0", dependencies = setOf(depCyc1)) | ||
| val depCyc2 = createDependency("org.cyclic", "cyclic", "77.7", dependencies = setOf(depFoo)) | ||
| // TODO: Enable this once the functionality to break cycles is fully implemented. | ||
| "deal with cycles in dependencies".config(enabled = false) { |
There was a problem hiding this comment.
"deal with cycles" is so unspecific, how about also changing it to "break cycles"?
| addDependencyToGraph(scopeName, it, transitive = true, processed) | ||
| if (it in processed) { | ||
| return@mapNotNullTo null | ||
| } |
There was a problem hiding this comment.
In #11207 I had this problem with this change:
The second commit tries to fix that issue by breaking cycles in transitive dependencies. With that fix, the first stack overflow does not happena anymore, but then there is a stack overflow in the default implementation of DependencyHandler.areDependenciesEqual when it tries to compare the dependency subtrees when adding the nodes.
Is this somehow solved by the early returns you added in the first commit?
There was a problem hiding this comment.
Is this somehow solved by the early returns you added in the first commit?
Yes, at least that fixed the stack overflow I was seeing in my PNPM experiments.
There was a problem hiding this comment.
there is a stack overflow in the default implementation of DependencyHandler.areDependenciesEqual
#11575 also added another safeguard in this regard.
sschuberth
force-pushed
the
deps-comp-imps
branch
3 times, most recently
from
8400556 to
758eb13
Compare
There is no cycle in the originally defined
depCyc2 -> depFoo -> depCyc1
dependency chain. Doing so is actually not directly possible with the
recursively defined `PackageReference` class.
Solve that by using a primitive `DependencyHandler` that just uses
indexes to identify dependencies.
The test passes thanks to the fix recently made in 80edb57.
Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
sschuberth
force-pushed
the
deps-comp-imps
branch
from
758eb13 to
25d7468
Compare
sschuberth
changed the title
3 participants
Please have a look at the individual commit messages for the details.