Low test coverage for dhall-to-{json,yaml,yaml-ng}

[sjakobi]

Bugs like #1559 and #1560 reveal that our current tests don't cover some very basic cases. Since conversion to JSON and YAML is such an important functionality, we should try to ensure that it works correctly.

[sjakobi]

I wonder whether we could set up some roundtrip tests: Generate a random Dhall expression, type-check it, convert it to JSON and YAML with random options, try to convert it back to Dhall with the corresponding options, check whether it's the original expression…

[sjakobi]

We could have another property test to check that dhall-to-yaml and dhall-to-yaml-ng behave the same…

[patrickmn]

That's a great idea. I feel like we just need to exhaust the space that JSON/Aeson can represent, do one alteration aeson-yaml/hsyaml options to match up the formatting, and be done with the inconsistencies.

I similarly wanted to use http://hackage.haskell.org/package/hedgehog-gen-json in the aeson-yaml test cases but it wasn't building at the time IIRC.

[sjakobi]

We already have generators that we could reuse in Dhall.Test.QuickCheck.

To make these available to dhall-json and dhall-yaml, I believe the most straightforward way would be to create (yet another!) new package dhall-property-tests(?) that would export the generators from its library and have the existing property tests in its testsuite.

Does this sound like the right approach @Gabriel439?

[sjakobi]

To make these available to dhall-json and dhall-yaml, I believe the most straightforward way would be to create (yet another!) new package dhall-property-tests(?) that would export the generators from its library and have the existing property tests in its testsuite.

@Gabriel439 what do you think about this plan?

[Gabriella439]

@sjakobi: Would that create a package cycle if the dhall package wanted to depend on these same generators?

One alternative could be for the main dhall package to depend on QuickCheck and export the Arbitrary instances itself

[Gabriella439]

Yet another option could be switching to a non-type-class-based property testing framework (e.g. hedgehog or something similar)

[sjakobi]

Would that create a package cycle if the dhall package wanted to depend on these same generators?

I would move dhall's property tests to the new package.

One alternative could be for the main dhall package to depend on QuickCheck and export the Arbitrary instances itself

One issue with that are the additional dependencies on QuickCheck, generic-random etc.. My other worry would be that downstream users might expect some stability in the PBT functionality, which I'd rather not promise at this stage.

Yet another option could be switching to a non-type-class-based property testing framework (e.g. hedgehog or something similar)

Yes, I think we'll want to move away from the type-class-based approach, in order to make it easier to tweak the generators depending on what we're testing.

Even then I believe we'd want to share some code between the various Expr-based property tests, and we'd still need to find a place for that shared code.

[Gabriella439]

My concern is that moving the tests to a separate package would create a slow feedback loop to discover issues caught by those tests when doing local development

[sjakobi]

In my experience the feedback loop is already fairly slow, since the QuickCheck module takes a while to compile, and becausge I usually have to generate thousands of test cases to get a failure.

How about we move the generators into the dhall library then and add a warning in the module that it is unstable and primarily for internal consumption? Dhall.Internal.PropertyTests?

If we create a dhall-core library (see #1102), we can consider moving the property test functionality behind a feature flag, to reduce the dependency footprint.

[Gabriella439]

@sjakobi: Yeah, I would be fine with that arrangement