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

Improving correctness #95

Open
sjakobi opened this issue Nov 4, 2019 · 9 comments
Open

Improving correctness #95

sjakobi opened this issue Nov 4, 2019 · 9 comments

Comments

@sjakobi
Copy link
Collaborator

sjakobi commented Nov 4, 2019

I'm quite impressed by (and a bit proud of!) the number of bugs that has been revealed through dhall's usage of prettyprinter!

I wonder how prettyprinter could rely less on the users to discover these bugs though.

In particular I wonder whether property tests could help – IMHO they've been quite effective in dhall!

For example I believe that the recent bugs could have been discovered by tests for the following properties:

  • Unbounded layout of grouped Line fails #91: The result of applying layout{Pretty,Smart} opts . group to a Doc that doesn't contain Fail, should never contain SFail.
  • For removeTrailingWhitespace:
    • The (rendered) input and output should only differ by trailing spaces.
    • The output should never contain trailing spaces.
    • removeTrailingWhitespace is idempotent.

It's probably more difficult to come up with properties that would reveal unknown bugs, but it might be worth trying!

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 4, 2019

Oh, I see that there is already an Arbitrary instance and a non-trivial property test:

fusionDoesNotChangeRendering :: FusionDepth -> Property
fusionDoesNotChangeRendering depth
= forAll document (\doc ->
let rendered = render doc
renderedFused = render (fuse depth doc)
in counterexample (mkCounterexample rendered renderedFused)
(render doc == render (fuse depth doc)) )
where
render = renderStrict . layoutPretty defaultLayoutOptions
mkCounterexample rendered renderedFused
= (T.unpack . render . vsep)
[ "Unfused and fused documents render differently!"
, "Unfused:"
, indent 4 (pretty rendered)
, "Fused:"
, indent 4 (pretty renderedFused) ]
newtype RandomDoc ann = RandomDoc (Doc ann)
instance Arbitrary (RandomDoc ann) where
arbitrary = fmap RandomDoc document

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 4, 2019

Here's a property for #83:

When layouting f (align (vsep […, x, …, x, …])), both instances of x should have the same layout.

@quchen
Copy link
Owner

quchen commented Nov 5, 2019

The main difficulty here is putting the properties into code, but I totally agree! The Arbitrary instance is not used very much right now, but I prefer Quickcheck to handwritten tests whenever possible.

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 6, 2019

Another property had come up in #89:

layoutWadlerLeijen fp opts (unAnnotate doc) === unAnnotateS (layoutWadlerLeijen fp opts doc)

But layoutCompact should be included too, so we probably want

layout (unAnnotate doc) === unAnnotateS (layout doc)

with layout being one of layout{Pretty,Smart,Compact} with arbitrary LayoutOptions.

sjakobi added a commit to sjakobi/prettyprinter that referenced this issue Nov 6, 2019
sjakobi added a commit to sjakobi/prettyprinter that referenced this issue Jan 20, 2020
@sjakobi
Copy link
Collaborator Author

sjakobi commented Jan 23, 2020

@1Jajen1 mentioned that he has some effective property tests in his kotlin clone.

Maybe we can copy some of those…

@1Jajen1
Copy link
Contributor

1Jajen1 commented Jan 23, 2020

@sjakobi Not sure that will work much better, currently I literally just generate random elements of Doc (without Union/Fail to preserve invariants although you could call group on them randomly to produce those as well). Shrinking is also done manually for most of it as I too use a quickcheck derivative for tests. I'll change those tests to more hedgehog style tests later this week to check out how well integrated shrinking works, it seems to be the better choice if you want to preserve invariants on more complex datatypes without writing shrinkers manually.

@sjakobi
Copy link
Collaborator Author

sjakobi commented Jan 23, 2020

@1Jajen1 Well somehow your tests were able to detect the problems with the modified group, right?

Which properties did you check there?

@1Jajen1
Copy link
Contributor

1Jajen1 commented Jan 23, 2020

So what I have right now is basically just a few property tests: (The kotlin source for these is actually readable!)
This one for the group (passes around 100k tests just fine, so it might actually work^^):
https://github.com/1Jajen1/kotlin-pretty/blob/master/kotlin-pretty/src/test/kotlin/pretty/GroupTest.kt
https://github.com/1Jajen1/kotlin-pretty/blob/master/kotlin-pretty/src/test/kotlin/pretty/Generators.kt
https://github.com/1Jajen1/kotlin-pretty/blob/master/kotlin-pretty/src/test/kotlin/pretty/DocTest.kt

They actually find a bug in flatten (did not handle Line correctly, only in the kotlin version).

@sjakobi
Copy link
Collaborator Author

sjakobi commented Jan 24, 2020

#126 (comment) lists a bunch of tests from dhall's testsuite, at least some of which we ought to adopt into prettyprinter's testsuite…

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

No branches or pull requests

3 participants