Larger keys for intmap and intset

[jwaldmann]

as discussed in #788 .

I have no idea on how to best pass the "-a N" option for testing. Does it go in haskell-ci.yml, or in cabal.haskell-ci? or in containers.cabal?

or in the source? That seems easiest to do, and most stable (independent from build/CI tooling) but it would ignore command-line options, which is bad? https://hackage.haskell.org/package/test-framework-0.8.2.0/docs/Test-Framework-Runners-Console.html#v:defaultMainWithArgs

Adding --test-option="-a 10000" does seem to increase CI build times - quite drastically. Or, they weren't very stable to begin with.

[sjakobi]

I have no idea on how to best pass the "-a N" option for testing. Does it go in haskell-ci.yml, or in cabal.haskell-ci? or in containers.cabal?

or in the source? That seems easiest to do, and most stable (independent from build/CI tooling) but it would ignore command-line options, which is bad? https://hackage.haskell.org/package/test-framework-0.8.2.0/docs/Test-Framework-Runners-Console.html#v:defaultMainWithArgs

So in dhall's testsuite we have a little helper that we use to specify the number of QuickCheck tests in the code, but that can still be overriden on the CLI with a higher number of tests:

https://github.com/dhall-lang/dhall-haskell/blob/8aed63714def462056847f685f5871d74c12b6cf/dhall/tests/Dhall/Test/QuickCheck.hs#L787-L791

We're using tasty though – I don't know whether something similar can be achieved with test-framework.

Since test-framework is basically unmaintained, and since tasty's API has a large overlap with test-framework, maybe we should migrate to tasty anyway?

[Bodigrim]

Migration to tasty usually boils down to renaming imports and wrapping top-level tests in testGroup “All”. The sooner we leave test-framework in the past, the better.

[sjakobi]

@treeowl How do you feel about migrating to tasty?

[treeowl]

I know nothing about test frameworks, so I'm very happy to let other people decide.

…

On Thu, Aug 5, 2021 at 11:45 AM Simon Jakobi ***@***.***> wrote: @treeowl <https://github.com/treeowl> How do you feel about migrating to tasty? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#790 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOOF7M3ODBK73O3NMNFTSDT3KW2PANCNFSM5A2K3OXA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

[treeowl]

Oh yeah, this discussion. Someone came up with something pretty decent (I think) for my little toy compact-sequences package. Does that look like something we could reasonably copy, or is it not up to the challenge of a big test suite?

[treeowl]

Now that I've merged #796, can you do what you want with test case sizes here? Ideally, I like to run tests three ways:

1. Many small cases.
2. A moderate number of moderately sized cases.
3. A small number of large cases.

The three ways should be tweaked to run in the same amount of time (give or take an order of magnitude or so). This should be almost as good as SmallCheck for the small cases, but also be likely to catch glaring mistakes in large ones.

[treeowl]

I'd feel more comfortable about this if we tested with multiple sizes. See treeowl/compact-sequences@585bfac for a commit doing so elsewhere.

[jwaldmann]

tested with multiple sizes

OK, I'll look into this.

Just for calibration, this (on my local machine)

time cabal test all --test-show-details=direct --test-options="--quickcheck-tests 1000 --quickcheck-max-size 100"

real    0m20.246s
user    1m18.343s
sys     0m4.309s

is about 3 min on CI.

Runtime does increase strongly with max-size.

[jwaldmann]

now the test run takes 25 min in CI (for each ghc version). that's too much?

[treeowl]

Ouch. Yeah, that seems excessive. Did you limit that to just IntMap and IntSet?

[jwaldmann]

no, I applied globally. jwaldmann@3a3929d

In theory, your method (lots of small, some large, etc.) should be fine for all tests?

[treeowl]

Yeah, it should work, but it's pricy. IntMap and IntSet seem much more likely than the other structures to exhibit special behavior around size extremes.

[jwaldmann]

I did restrict the test sets that get larger parameters. It's still expensive, and the programming gets ugly. I am not an expert at tasty's not-quite-regexps, and CI's not-quite-bash.

OTOH, perhaps 20 min (per GHC version) isn't too bad. If it catches bugs? The contributor can run faster tests locally. The maintainer needs days to accept the PR, anyway. Which is fine. (Though, a more clever CI would use that waiting time to run extra tests ... or even write some...)

[treeowl]

Looking over the conversation, it's not clear whether everything is resolved. Could you possibly update?

[jwaldmann]

I'm on it (just did a rebase).

I wanted to take up #787 again. I was benchmarking NFA-DFA conversion with several Map IntSet foo implementations (hint: HashMap wins, Data.Map seems close, but only with the non-allocation Ord instance. I think.)

[EDIT] see https://gitlab.imn.htwk-leipzig.de/waldmann/fdaln/-/blob/master/DFA.hs

[meooow25]

The large keys part of this is obsolete (#788 (comment)), but this PR also bumps the quickcheck args. Not sure if we still want to do that?

[meooow25]

Closing for now. Bumping the number of quickcheck tests seems like a good idea, so we can still do that. It will probably need to be configured via the cabal.haskell-ci file.