search

haskell / random

Random number library
Other
53 stars 50 forks source link

Inspection testing? #102

Closed Bodigrim closed 3 years ago

Bodigrim commented 3 years ago

How do we feel about inspecting Core in our tests by means of inspection-testing? This would for example allow to catch inlining regression in GHC 9.0 from #100.

lehins commented 3 years ago

Personally I find benchmarks work best for catching performance regressions, which is exactly what caught them in #100 :wink: That being said I do still think that inspection testing is a great idea, but it would require someone actually writing those tests. So, if you are up to it, knock yourself out.

Shimuuar commented 3 years ago

I think it's a great idea. Only problem is finding good invariants.

bgamari commented 3 years ago

Thank you, @Bodigrim! As you know, I am very much a fan of inspection-testing. It is much easier to sort out why an inspection-testing test is failing than it is to sift through piles of Core and assembler after a benchmark regression.

Bodigrim commented 3 years ago

I've actually caught an issue with inlining of Generic-derived Uniform instances. This is probably a good sign :)

Bodigrim commented 3 years ago

I do not quite understand why lts-14 build was fine in PR, but fails when merged. Might it be a caching issue?..

lehins commented 3 years ago

@Bodigrim sorry didn't have time for OSS work during last month. Vacation and job change took all my focus away.

I do not quite understand why lts-14 build was fine in PR, but fails when merged.

As it turns out caching was not at fault. It is building with coverage that is causing the inspection tests to fail. LTS-14 is the only one that runs the coverage report and uploads to coveralls, that's why it didn't fail for PRs, but did when was merged. I've changed the CI script a bit to ensure that PRs go through the same build/test path. I'll open an issue about this, so we can discuss the best approach to solve/work around this inspection testing issue