Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Low test coverage of normalizeWithM #1114

Closed
sjakobi opened this issue Jul 15, 2019 · 2 comments · Fixed by #1126
Closed

Low test coverage of normalizeWithM #1114

sjakobi opened this issue Jul 15, 2019 · 2 comments · Fixed by #1126

Comments

@sjakobi
Copy link
Collaborator

sjakobi commented Jul 15, 2019

If I wrap the entire implementation of Dhall.Core.normalizeWithM with an undefined, only 8 tests fail:

Dhall Tests
  normalization
    customization
      simpleCustomization:                                                                      FAIL
        Exception: Prelude.undefined
        CallStack (from HasCallStack):
          error, called at libraries/base/GHC/Err.hs:78:14 in base:GHC.Err
          undefined, called at src/Dhall/Core.hs:1297:25 in dhall-1.24.0-Hhvu3RMIrBsIeoMyZcG2FL:Dhall.Core
      doubleReduction:                                                                          FAIL
        Exception: Prelude.undefined
        CallStack (from HasCallStack):
          error, called at libraries/base/GHC/Err.hs:78:14 in base:GHC.Err
          undefined, called at src/Dhall/Core.hs:1297:25 in dhall-1.24.0-Hhvu3RMIrBsIeoMyZcG2FL:Dhall.Core
  tutorial
    Functions
      Example #0:                                                                               FAIL
        Exception: Prelude.undefined
        CallStack (from HasCallStack):
          error, called at libraries/base/GHC/Err.hs:78:14 in base:GHC.Err
          undefined, called at src/Dhall/Core.hs:1297:25 in dhall-1.24.0-Hhvu3RMIrBsIeoMyZcG2FL:Dhall.Core
      Example #1:                                                                               FAIL
        Exception: Prelude.undefined
        CallStack (from HasCallStack):
          error, called at libraries/base/GHC/Err.hs:78:14 in base:GHC.Err
          undefined, called at src/Dhall/Core.hs:1297:25 in dhall-1.24.0-Hhvu3RMIrBsIeoMyZcG2FL:Dhall.Core
      Example #2:                                                                               FAIL
        Exception: Prelude.undefined
        CallStack (from HasCallStack):
          error, called at libraries/base/GHC/Err.hs:78:14 in base:GHC.Err
          undefined, called at src/Dhall/Core.hs:1297:25 in dhall-1.24.0-Hhvu3RMIrBsIeoMyZcG2FL:Dhall.Core
  Input
    Handle union literals
      Pass through:                                                                             FAIL
        Exception: Prelude.undefined
        CallStack (from HasCallStack):
          error, called at libraries/base/GHC/Err.hs:78:14 in base:GHC.Err
          undefined, called at src/Dhall/Core.hs:1297:25 in dhall-1.24.0-Hhvu3RMIrBsIeoMyZcG2FL:Dhall.Core
      Pass through:                                                                             FAIL
        Exception: Prelude.undefined
        CallStack (from HasCallStack):
          error, called at libraries/base/GHC/Err.hs:78:14 in base:GHC.Err
          undefined, called at src/Dhall/Core.hs:1297:25 in dhall-1.24.0-Hhvu3RMIrBsIeoMyZcG2FL:Dhall.Core
      Pass through:                                                                             FAIL
        Exception: Prelude.undefined
        CallStack (from HasCallStack):
          error, called at libraries/base/GHC/Err.hs:78:14 in base:GHC.Err
          undefined, called at src/Dhall/Core.hs:1297:25 in dhall-1.24.0-Hhvu3RMIrBsIeoMyZcG2FL:Dhall.Core

8 out of 1200 tests failed (4.75s)

If I insert undefineds in just some of the Expr alternatives, the testsuite isn't affected at all.

Given that we have some fairly complex logic in normalizeWithM, I wonder whether some of it is already broken.

To increase test coverage, could we simply run the normalization tests through normalizeWithM too? The "trivial" Normalizer appears to be simply (Identity . Just).

A assume we cannot simply get rid of normalizeWithM because someone's actually using it?!

@sjakobi
Copy link
Collaborator Author

sjakobi commented Jul 15, 2019

The "trivial" Normalizer appears to be simply (Identity . Just).

Ah, no. It's probably something like \_e -> Identity Nothing.

@Gabriella439
Copy link
Collaborator

@sjakobi: We really only need one test, which is just a property test that ensures that normalizeWithM behaves the same as normalize when using the Identity Monad

sjakobi added a commit that referenced this issue Jul 18, 2019
* Implements constant folding of Natural/fold applications normalizeWithM.

* Changes the Arbitrary Var instance to generate only non-negative indices.
  Otherwise failures like this one would pop up:

    normalizeWithM should be consistent with normalize: FAIL (8.51s)
          *** Failed! Falsified (after 318133 tests and 6 shrinks):
          Let (Binding {variable = "", annotation = Nothing, value = List} :| []) (Var (V "" (-1)))
          Var (V "" (-1)) /= Var (V "" (-2))
          Use --quickcheck-replay=180244 to reproduce.

Fixes #1114.
@mergify mergify bot closed this as completed in #1126 Jul 19, 2019
mergify bot pushed a commit that referenced this issue Jul 19, 2019
* Check normalizeWithM for consistency with normalize

* Implements constant folding of Natural/fold applications normalizeWithM.

* Changes the Arbitrary Var instance to generate only non-negative indices.
  Otherwise failures like this one would pop up:

    normalizeWithM should be consistent with normalize: FAIL (8.51s)
          *** Failed! Falsified (after 318133 tests and 6 shrinks):
          Let (Binding {variable = "", annotation = Nothing, value = List} :| []) (Var (V "" (-1)))
          Var (V "" (-1)) /= Var (V "" (-2))
          Use --quickcheck-replay=180244 to reproduce.

Fixes #1114.

* QuickCheck tests: Specialize the 'natural' Gen to Naturals

Previously it could produce about any number, not just non-negative
ones.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants