test(model): Fix the test for cyclic dependencies

[sschuberth]

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.

[sschuberth]

@oheger-bosch, fixing the test for cyclic dependencies now actually shows that we're running into a stack overflow.

From a first glance it seems that calling breakCycles() / removeCycles() happens too late, as the recursion already happens in addDependencyToGraph() called by DependencyGraphBuilder.addDependency() before DependencyGraphBuilder.build() is called.

Can you have a look as well?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.60%. Comparing base (f26c647) to head (2d18c93).

Additional details and impacted files

@@            Coverage Diff            @@
##               main   #10083   +/-   ##
=========================================
  Coverage     69.60%   69.60%           
  Complexity     1454     1454           
=========================================
  Files           270      270           
  Lines          9668     9668           
  Branches       1028     1028           
=========================================
  Hits           6729     6729           
  Misses         2487     2487           
  Partials        452      452           

Flag	Coverage Δ
funTest-non-docker	33.32% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[oheger-bosch]

It is unclear to me what you want to achieve with this PR.

The code that adds dependencies to the graph uses a set to store the nodes it already has processed and to detect cycles in the graph. Obviously, your artificial dependency handler implementation tricks this out. Not sure how realistic this is.

[sschuberth]

It is unclear to me what you want to achieve with this PR.

I'm currently inspecting various "strange" real-world issues that we have when analyzing Gradle or Node projects (like #9763). As part of that I was inspecting various areas of the code again, and came across this test, which does not seem to be doing what it advertises.

But do you agree that the test in its current form actually does not check that cycles are properly handled?

Obviously, your artificial dependency handler implementation tricks this out. Not sure how realistic this is.

This handler is modeled after a real-world handler where the identifiers for packages are just strings, and in order to determine transitive dependencies you need to use this string in a map of packages where each string is mapped to its direct dependencies.

[sschuberth]

But do you agree that the test in its current form actually does not check that cycles are properly handled?

@oheger-bosch, I'd appreciate some feedback here.

[oheger-bosch]

But do you agree that the test in its current form actually does not check that cycles are properly handled?

@oheger-bosch, I'd appreciate some feedback here.

Well, the test handles at least a logic cyclic dependency between some packages; although this does not seem to be a cycle in the form that you encounter. Given that this code has been developed in a test-driven style, there must have been a stage in the implementation in which this test was failing.
Maybe rename it to make the difference clearer?

[sschuberth]

Maybe rename it to make the difference clearer?

I'd appreciate any proposals for titles then, as to me it's totally unclear what this test is supposed to cover, that is not yet covered by other tests.

[oheger-bosch]

Maybe rename it to make the difference clearer?

I'd appreciate any proposals for titles then, as to me it's totally unclear what this test is supposed to cover, that is not yet covered by other tests.

If you want to remove it, I won't object.

[sschuberth]

If you want to remove it, I won't object.

Hmm. IMO we should either make the title match the test code, or make the test code match the title. The latter is what I've originally proposed here, to gain coverage for that case. Somehow this discussion is also going in cycles 😉

[oheger-bosch]

Hmm. IMO we should either make the title match the test code, or make the test code match the title. The latter is what I've originally proposed here, to gain coverage for that case. Somehow this discussion is also going in cycles 😉

Sorry, this was not my intention.

To move this forward, it is probably necessary to figure out whether there is actually a problem with such cyclic dependencies you create in your modified test (and the stack overflow seems to indicate this). So, do you have any idea why the cycle detection does not work here? The insertIntoGraph function of DependencyGraphBuilder has this set of already processed nodes. So, why does it still run in an infinite loop?

[sschuberth]

So, do you have any idea why the cycle detection does not work here?

Please see my comment above: My assumption was that cycles are not detected / broken unless breakCycles() / removeCycles() is called, which happens too late.

Or are you saying that cycle detection and breaking of cycles happens at different stages?

The insertIntoGraph function of DependencyGraphBuilder has this set of already processed nodes. So, why does it still run in an infinite loop?

I'm not sure (how it's supposed to work exactly). The only place where the set of processed nodes is amended is here, but that code is never hit when running the test. I'd appreciate if you could try to debug the new test as well, as you are way more familiar with the code than I am.