Running regression tests against one backend.

[christiaanb]

In https://github.com/clash-lang/clash-compiler/pull/1936/files#diff-53bdb3d095d1977ff127ea7945cf60f53f260192d6f821cf5feb2c86c65e4b4bR43-R50 we added a regression test against all three backends, which I should note is something that we've been doing by default, even though the original bug was backend-agnostic.

To save on CI time, I wonder whether we should just run these backend-agnostic regression tests against a single backend. This is something I already did in e.g. https://github.com/clash-lang/clash-compiler/pull/1925/files#diff-9270575e760574273e144931e9ae5e7f544cc12deb4ebc44c17dc93d348f9f30R535

Thoughts?

[alex-mckenna]

If it's truly agnostic I wonder if running it against a backend which is randomly chosen per test is a good idea. That should give a good compromise between testing regressions on different backends while not spending a (hopefully) unnecessary time in CI. It also means we can avoid the trap of testing all our regressions on (for instance) VHDL and not noticing as quickly if regressions get into Verilog.

Another point to consider is what if the same failure is caused by different defects in different HDLs special code paths. In that case there's an argument for wanting to run the regression test on all backends because there are multiple disjoint fixes to check for regression...

Maybe a solution would be to have test target as something like

data TestTarget = Any [Hdl] | All [Hdl]

where Any tests one of the backends (chosen at random) and All tests all backends

[alex-mckenna]

True, although I'd hope the risk would be small if Any is used correctly. A downside is that if something that was previously Any [VHDL, Verilog] gets a special code path for Verilog, then it should also be changed to All [VHDL, Verilog], which is easily forgotten. There's also the downside of determining how tests we currently have should be marked. Simply having

testCommon = ...
testVHDL = testCommon VHDL
testVerilog = testCommon Verilog

doesn't necessarily mean the fix for each backend was the same, just that the test is the same.

[alex-mckenna]

Another point: is it worse to see sporadic unrelated failures in PRs, or to not see these failures at all? While more annoying, I would still lean towards the former. If it's from an external contributor then a maintainer can always step in and fix it, but it does slow down the PR getting merged

[alex-mckenna]

Perhaps we could have have a "paranoid mode" which runs on nightly and treats Any as if it were All? I'm not really sure where the best compromise is though

[leonschoorl]

I like the Any/All idea too.

For the current tests we might just mark them all with All, to keep the current testsuite behavior.
And selectively mark them Any for particularly slow tests, as we create new tests or as we run into them.

[alex-mckenna]

I think given the amount of tests we have it might be worth taking the time to figure it out where possible (because the potential for saving CI time seems to be pretty reasonable for the effort). Maybe we should limit the tests we look at changing to Any to just ones with a clear provenance (i.e. those with names that refer to issue numbers)

[leonschoorl]

Good point
I had similar ideas when I wrote that test, but ignored them.
Would be good to pick this up at some point.

[leonschoorl]

Some other point related to tests and HDLs:
We currently don't run any SystemVerilog tests on CI because we don't have a simulator for it.
But we could run netlist and output tests for SystemVerilog without a simulator if we wanted.

[alex-mckenna]

We could also test SystemVerilog for anything where hdlLoad and hdlSyn are both False right? Although I assume this isn't that many tests (if any)

[martijnbastiaan]

I'm very supportive of trying to cut down on CI times. I'm hesitant to support selectively running tests though, as it seems to work around the real problem: 95% of the testsuite's time is spent on spinning up Clash / the GHC API. Furthermore, I'm worried that this will lead to friction both for us and new users; flaky CI is no fun.

Given that almost no time is spent in normalization / netlist generation, but in parsing/typechecking/loading interface files I'd propose exploring three routes:

• Moving tests to exposed-modules here:

  clash-compiler/tests/clash-testsuite.cabal

  Lines 97 to 98 in 0415bcf

  	-- From tests/shouldwork/LoadModules
  	T1796

  . This would prevent GHC from compiling the same files over and over. An additional benefit would be a proper test hierarchy and quick feedback on API changes, as cabal repl clash-testsuite can be used. Lastly, with the starter project and clash libraries becoming a thing now, most code in the wild that Clash compiles will be loaded from interface files. Hence, doing this in the testsuite would more accurately mimic how Clash is used by our users. Biggest obstacle here is that not all tests can be moved here, as some rely behavior not present when definitions are loaded from interface files. I'm also guessing that the speed benefits are relatively minor.

• Adding support for generating multiple HDLs in one go. As almost all test benches are trivial to normalize and netlistify, we could win quite some time by supporting "loading code" and then running normalization and netlist three times.

• More generally, Clash should probably be able to reuse the same GHC session (and loaded modules) for multiple runs. Use Tasty's withResource and a resource pool containing Clash sessions, we should be able to cut down the time spent in clash-testsuite by an order of magnitude.