Use Diff package instead diff program to test cabal-to-dhall

[jneira]

As commented in #72

[ocharles]

Can you confirm invalid tests produce useful output? Attaching example output here would help me evaluate.

[jneira]

The Diff output is similar to the utility one, for example modifying the simple.cabal test case with:

cabal-version: 2.0
name: test
version: 1.0
author: author

The output of the failing tests is:

Diff between expected golden-tests/cabal-to-dhall/simple.dhall and actual golden-tests/cabal-to-dhall/simple.cabal :
6c6
<         ""
---
>         "author"

Maybe the comment should be more precise cause is the difference with the dhall file generated from cabal one.

[jneira]

Anyway let me replicate the output for goldenVsStringDiff so the difference with previous version only would be the style of diff output (Diff output is like standard diff without the -u flag)

[quasicomputational]

Is there a reason why this code is written differently to how dhall-to-cabal's test is? I would have thought that the problems are the same (compare an abstract representation in memory to a serialised representation on disk), so the code ought to look the same.

[jneira]

@quasicomputational mmm, dhall-to-cabal uses the GenericPackageDescription parsed from both files to test so it not depends on formatting. To get the same behaviour for cabal-to-dhall, i think we should compare the generated dhall ast from both sources instead the pretty printed output text of cabalToDhall

[quasicomputational]

I think we should. dhall 1.15 is going to change the formatting of its output (see #77's diffstat for an indication of the damage), so if we can test for equality on the abstract trees and then diff on the concrete syntax it'd be good.

I agree with your earlier worry that the error message isn't clear that the diff is comparing to the re-serialised golden file rather than the exact representation on disk, but that's actually already a problem in the dhall-to-cabal test, so I guess it shouldn't block merging this and we can fix that later.

[jneira]

With the last commit the diff output is similar to goldenVsStringDiff

• Previous output:

dhall-to-cabal-1.1.0.0: test (suite: golden-tests)

golden tests
  dhall-to-cabal
    SPDX:                     OK (17.51s)
    nested-conditions:        OK (19.53s)
    gh-55:                    OK (11.00s)
    gh-53:                    OK (12.52s)
    empty-package:            Diff between expected golden-tests/dhall-to-cabal/
empty-package.cabal and actual golden-tests/dhall-to-cabal/empty-package.dhall :

4a5
> author: author

FAIL (0.89s)
      Generated .cabal file does not match input
    dhall-to-cabal:           OK (54.37s)
    conditional-dependencies: OK (18.91s)
    compiler-options-order:   OK (14.79s)
  cabal-to-dhall
    SPDX:                     OK (0.15s)
    simple:                   FAIL (0.03s)
      Test output was different from 'golden-tests/cabal-to-dhall/simple.dhall'.
 Output of ["diff","-u","golden-tests/cabal-to-dhall/simple.dhall","C:\\TEMP\\si
mple.dhall5852-1.actual"]:
      --- golden-tests/cabal-to-dhall/simple.dhall      2018-06-05 14:47:20.5741
18800 +0200
      +++ C:\TEMP\simple.dhall5852-1.actual     2018-06-06 09:20:00.875802800 +0
200
      @@ -3,7 +3,7 @@
       in  let types = ../../dhall/types.dhall

       in  { author =
      -        ""
      +        "author"
           , benchmarks =
               [] : List { benchmark : types.Config → types.Benchmark, name : Te
xt }

           , bug-reports =
    otheros:                  OK (0.04s)
    gh-36:                    OK (0.06s)
    conditional-dependencies: OK (0.04s)

2 out of 13 tests failed (149.88s)

dhall-to-cabal-1.1.0.0: Test suite golden-tests failed

• Actual output:

dhall-to-cabal-1.1.0.0: test (suite: golden-tests)

golden tests
  dhall-to-cabal
    SPDX:                     OK (18.01s)
    nested-conditions:        OK (19.84s)
    gh-55:                    OK (11.38s)
    gh-53:                    OK (12.53s)
    empty-package:            FAIL (0.92s)
      Test output was different from 'golden-tests/dhall-to-cabal/empty-package.
cabal'.
      Output of diff between 'golden-tests/dhall-to-cabal/empty-package.cabal' a
nd test output using 'golden-tests/dhall-to-cabal/empty-package.dhall':
      4a5
      > author: author
    dhall-to-cabal:           OK (55.44s)
    conditional-dependencies: OK (19.29s)
    compiler-options-order:   OK (15.22s)
  cabal-to-dhall
    SPDX:                     OK
    simple:                   FAIL
      Test output was different from 'golden-tests/cabal-to-dhall/simple.dhall'.

      Output of diff between 'golden-tests/cabal-to-dhall/simple.dhall' and test
 output using 'golden-tests/cabal-to-dhall/simple.cabal':
      6c6
      <         ""
      ---
      >         "author"
    otheros:                  OK
    gh-36:                    OK (0.05s)
    conditional-dependencies: OK (0.01s)

2 out of 13 tests failed (152.73s)

dhall-to-cabal-1.1.0.0: Test suite golden-tests failed

[jneira]

As the diff ouput can be useful although we change the test check test in cabal-to-dhall test group, i think we could include this version and later change the check to compare the dhall ast's

[jneira]

@ocharles what do you think about merge it as is?

[ocharles]

Hi - I've been at ZuriHac all weekend and haven't had a chance to review yet. I'll try and review it this week!

…

On Sun, 10 Jun 2018, 8:39 pm Javier Neira, ***@***.***> wrote: @ocharles <https://github.com/ocharles> what do you think about merge it as is? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#90 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABRjtR-Q-7wKBTgZKNfPXm9TyM4VTJ5ks5t7WfjgaJpZM4UbbL5> .

[ocharles]

To get the same behaviour for cabal-to-dhall, i think we should compare the generated dhall ast from both sources instead the pretty printed output text of cabalToDhall

I think we should do this. https://hackage.haskell.org/package/dhall-1.14.0/docs/Dhall-Diff.html is already doing the hard work for us, it just didn't exist when we first wrote these tests.

[quasicomputational]

Looks sensible to me (still). I think I'd merge this without updating the tests to Dhall 1.19 and leave that to #143, but it's not really a big difference either way.

[jneira]

Ok, i've re-rebased this one with master

[ocharles]

Slightly reformatted but looks good to me! Will merge once CI is happy. Thank you!