UnkindPartition / tasty

Modern and extensible testing framework for Haskell
638 stars 108 forks source link

Print dependency cycles in error message #340

Closed martijnbastiaan closed 2 years ago

martijnbastiaan commented 2 years ago

While Tasty detects cycles and prints an error message, it does not print the cycles it found. This PR makes sure it does. I've tested it on another code base with cycles in it, the output looks like:

clash-testsuite: Test dependencies form a loop:

..tests.shouldwork.Issues.T2046.VHDL.Vivado => ..tests.shouldwork.Issues.T2046.VHDL.Vivado

..tests.shouldwork.Issues.T2046.SystemVerilog.Vivado => ..tests.shouldwork.Issues.T2046.SystemVerilog.Vivado

..tests.shouldwork.BlackBox.T2117.VHDL.Vivado => ..tests.shouldwork.BlackBox.T2117.VHDL.Vivado

..tests.shouldfail.Verification.NonTemporalPSL.VHDL.Vivado => ..tests.shouldfail.Verification.NonTemporalPSL.VHDL.Vivado

Note the double dot is an artifact of this particular test tree.

martijnbastiaan commented 2 years ago

Sorry, goofed up. The PR should now pass CI as demonstrated here.

martijnbastiaan commented 2 years ago

I've added a changelog entry. @Bodigrim Given that this PR has been open for two weeks and hasn't attracted any comments, I doubt it is controversial. Is there anything I can do to help this get merged?

Bodigrim commented 2 years ago

I think this is a valuable addition, which justifies a minor breakage. Indeed I needed such feature in the past. @andreasabel @ocharles @VictorCMiraldo @adamgundry opinions?

VictorCMiraldo commented 2 years ago

I never needed this and I didn't know that tasty does detect dependency cycles. Makes sense and I can see how this can useful. The only think I'd suggest is shortening the output, if we have a dependency loop [a,b,c,d], why not print names only once? For instance:

Cyclic test dependencies:
  - a
  - b
  - c
  - d
martijnbastiaan commented 2 years ago

I did that to make self-loops non-ambiguous / more obvious to read. In my example I've got four separate dependency cycles, but they're all forming a self-loop. If we'd go with the list as you posted, I think we should do something like:

Cyclic test dependencies:

Cycle 1:
  - a
  - b
  - c

Cycle 2:
  - d
martijnbastiaan commented 2 years ago

@VictorCMiraldo I've implemented your suggestion. Could you re-review and/or start CI?

VictorCMiraldo commented 2 years ago

Thank you @martijnbastiaan, I triggered CI. @Bodigrim, as you mentioned, this is a breaking change. That being said, I agree that it is very unlikely to break for many users and @martijnbastiaan already prepped the changelog with a relevant entry for a future release of tasty. Do you think this still needs anything or should we merge once CI goes green?

martijnbastiaan commented 2 years ago

Thanks @Bodigrim, I've added a test for it too. Should be good now!

Bodigrim commented 2 years ago

@VictorCMiraldo feel free to merge.

VictorCMiraldo commented 2 years ago

Let's merge then! :)