feat(dhall-docs): Add test-setup and assertions as examples #1899

german1608 · 2020-07-06T01:22:15Z

Hi there!

First things first: I apologize about not being so active this (almost ended) week. I had to finish some things from my university that needed to be done for this next week, so I had to take some time from this to do them.

I'll try to brief all the details that this (kind of big) PR includes.

The most important thing is the test-setup. I decided to go with a golden-test setup using tasty-silver similar to what we've done on dhall-{json,yaml}. Basically:

There is a package folder at dhall-docs/tasty/data/package
The golden files are located at dhall-docs/tasty/data/golden
If the dhall file [path]/foo.dhall is on the package folder, the [path]/foo.dhall.html is on the golden folder.
These tests are most to check that we didn't break anything, not to do some kind of TDD with them since it is not viable to write the golden files by hand because lucid doesn't provide a way to render html as text in a non-minified way (check the golden files for a example).
The --accept flag is the best thing people invented. It not only updates the files, but create new ones if the file is missing. This is useful to add new tests by adding a new file on the package folder.
The whole setup fitted under 57 lines and we probably won't have to modify anymore, just add new dhall files on the package folder.

... but before doing this whole test-setup, I had to basically rewrite the whole logic for document generation (thanks god I splitted Html () generation from there). Specifically:

I separated the pure and impure parts from the Document generation. The core logic to generate documentation is IO free
I used a Writer monad to properly handle warnings.
The most trickiest part was the createIndexes function, probably it can be improved and moreover it may be difficult to understand.
The whole API for the Core.hs module (and basically all the dhall-docs one) is generateDocs, generatedDocsPure and GeneratedDocs

...and to end, I added assertions as examples on the test files.

I need to say that the new code, although it was difficult to get it right, looks way cleaner from my perspective. Also, tests adds a lot of value to dhall-docs, and I'd like to thanks @sjakobi for the suggestion to do this.

sjakobi · 2020-07-06T02:36:27Z

Oh, this looks very exciting! I'll review it properly soon. For now, I'm wondering whether the golden tests could include an HTML beautifier step, so the golden files are more human-readable. That should also help with understanding changes to the test output. You could just run --accept and then git diff to see the changes.

First things first: I apologize about not being so active this (almost ended) week. I had to finish some things from my university that needed to be done for this next week, so I had to take some time from this to do them.

No worries at all, @german1608! :)

german1608 · 2020-07-06T02:39:58Z

Oh, this looks very exciting! I'll review it properly soon.

Sure! Take your time, this PR is quite large :)

For now, I'm wondering whether the golden tests could include an HTML beautifier step, so the golden files are more human-readable. That should also help with understanding changes to the test output. You could just run --accept and then git diff to see the changes.

from lucid package there is no html beautifier, and implementing one by hand will take a lot of work to get it right, and it's outside of the purpose of dhall-docs.

I'll checkout later if there is a beautifier in some other package that is not bound to the Html () data type from lucid, maybe something Text -> Text like.

dhall-docs/src/Dhall/Docs/Core.hs

Gabriella439 · 2020-07-06T15:53:13Z

Also, Hydra is failing with this error:

cabal: filepath wildcard 'tasty/data/golden/*.html' does not match any files.

https://hydra.dhall-lang.org/jobset/dhall-haskell/1899#tabs-errors

It looks like the extra-source-files section needs to be updated to remove that glob

german1608 · 2020-07-06T17:50:18Z

Also, Hydra is failing with this error:
cabal: filepath wildcard 'tasty/data/golden/*.html' does not match any files.
https://hydra.dhall-lang.org/jobset/dhall-haskell/1899#tabs-errors

It looks like the extra-source-files section needs to be updated to remove that glob

Mm weird because that's the path of the golden files, which are included

german1608 · 2020-07-06T21:13:03Z

@sjakobi I noticed that mmark, which outputs Html (), already prints each element in a separate line in the rendered html.

I was reading its code and it turns out it adds a newline ("\n" :: Html()) after each element. I guess that will work, although it adds that overhead that we should remind to add it every timme.

sjakobi · 2020-07-06T21:46:41Z

@sjakobi I noticed that mmark, which outputs Html (), already prints each element in a separate line in the rendered html.

I was reading its code and it turns out it adds a newline ("\n" :: Html()) after each element. I guess that will work, although it adds that overhead that we should remind to add it every timme.

Yeah. I don't really want to make the library code less readable and slower than necessary.

I've been googling for a HTML prettyprinter in Haskell but without much success. I think the most promising thing I found is the HaXml package. It offers an HTML parser and a prettyprinter. I probably wouldn't want to use it in the library, but it might be good enough to improve the test output. Could you give it a quick spin?

german1608 · 2020-07-06T22:07:52Z

I've been googling for a HTML prettyprinter in Haskell but without much success.

Same here

I think the most promising thing I found is the HaXml package. It offers an HTML parser and a prettyprinter. I probably wouldn't want to use it in the library, but it might be good enough to improve the test output. Could you give it a quick spin?

Sure, I'll give it a try

german1608 · 2020-07-06T22:38:38Z

@sjakobi I did what you suggest and although it isn't the best prettyprinter, it is way more readable than a single line so I'll keep it, thanks for your help!

@Gabriel439 (or maybe @sjakobi), would you help me with the issue in hydra with the bad glob pattern? I don't get why it is wrong, tried locally and it's the same.

german1608 · 2020-07-06T22:42:51Z

@sjakobi I did what you suggest and although it isn't the best prettyprinter, it is way more readable than a single line so I'll keep it, thanks for your help!

Indeed it outputs <script> tags wrong, although for practical cases it's ok for us

sjakobi · 2020-07-06T22:53:18Z

@Gabriel439 (or maybe @sjakobi), would you help me with the issue in hydra with the bad glob pattern? I don't get why it is wrong, tried locally and it's the same.

cabal: filepath wildcard 'tasty/data/golden/*.html' does not match any files.

Yeah, that's strange. It should match dhall-docs/tasty/data/golden/index.html, right?

@sjakobi I did what you suggest and although it isn't the best prettyprinter, it is way more readable than a single line so I'll keep it, thanks for your help!

Indeed it outputs <script> tags wrong, although for practical cases it's ok for us

Oof, yeah. The output isn't exactly beautiful. ;)

What's the problem with the <script> tags?

sjakobi · 2020-07-06T23:02:00Z

@Gabriel439 (or maybe @sjakobi), would you help me with the issue in hydra with the bad glob pattern? I don't get why it is wrong, tried locally and it's the same.

One way to work around it would probably be to use Cabal >= 2.4: https://cabal.readthedocs.io/en/latest/file-format-changelog.html#cabal-version-2-4

Could we simply turn off hydra builds for dhall-docs with GHC 8.4?

german1608 · 2020-07-06T23:23:34Z

@Gabriel439 (or maybe @sjakobi), would you help me with the issue in hydra with the bad glob pattern? I don't get why it is wrong, tried locally and it's the same.
cabal: filepath wildcard 'tasty/data/golden/*.html' does not match any files.
Yeah, that's strange. It should match dhall-docs/tasty/data/golden/index.html, right?

Yes it should

What's the problem with the <script> tags?

<script> tags must always have its closer tag even when it's empty:

<script ... ></script>

The following is invalid, although there are other html tags that supports it:

<script />

One way to work around it would probably be to use Cabal >= 2.4: https://cabal.readthedocs.io/en/latest/file-format-changelog.html#cabal-version-2-4

Could we simply turn off hydra builds for dhall-docs with GHC 8.4?

dhall-docs only builds on hydra with GHC 8.6.1. Let me check those docs

german1608 · 2020-07-06T23:39:51Z

Where can I see what cabal version is nix using?

german1608 · 2020-07-06T23:49:37Z

Further debugging guided me to the following.

First, *.html matches foo.html but not foo.bar.html, and we have .dhall.html files, so I separated the glob patterns using those:

    tasty/data/golden/*.dhall.html
    tasty/data/golden/a/*.dhall.html
    tasty/data/golden/a/b/*.dhall.html
    tasty/data/golden/a/b/c/*.dhall.html
    tasty/data/golden/deep/nested/folder/*.dhall.html
    tasty/data/golden/*.html
    tasty/data/golden/a/*.html
    tasty/data/golden/a/b/*.html
    tasty/data/golden/a/b/c/*.html
    tasty/data/golden/deep/nested/folder/*.html

then I see that it didn't failed on the .dhall.html files, only on .html.

I change *.html to index.html;

    tasty/data/golden/*.dhall.html
    tasty/data/golden/a/*.dhall.html
    tasty/data/golden/a/b/*.dhall.html
    tasty/data/golden/a/b/c/*.dhall.html
    tasty/data/golden/deep/nested/folder/*.dhall.html
    tasty/data/golden/index.html
    tasty/data/golden/a/index.html
    tasty/data/golden/a/b/index.html
    tasty/data/golden/a/b/c/index.html
    tasty/data/golden/deep/nested/folder/index.html

and it still fails, with an error from openFile (does not exist). I'm researching the reason

german1608 · 2020-07-07T01:20:17Z

Definetly it's an issue with index.html files. I added a test file at tasty/data/index.html with its glob pattern in dhall-docs.cabal and it didn't detect but if I rename index.html to anything else it works.

german1608 · 2020-07-07T01:21:14Z

I think that as workaround I can manually intercept generated index.html and rename it to anything else. Any filename that doesn't ends in .dhall.html is a safe value.

sjakobi · 2020-07-07T11:24:10Z

Definetly it's an issue with index.html files. I added a test file at tasty/data/index.html with its glob pattern in dhall-docs.cabal and it didn't detect but if I rename index.html to anything else it works.

Strange! Have you worked out where the problem with the index.html arises? Is it a problem with Cabal or does it come from the Nix toolchain?

dhall-docs/dhall-docs.cabal

dhall-docs/src/Dhall/Docs/Core.hs

dhall-docs/tasty/data/package/InvalidMarkdown.dhall

sjakobi · 2020-07-07T11:59:52Z

dhall-docs/src/Dhall/Docs/Core.hs

                [] -> Nothing
                binding : _ -> Just binding

-{-| Calculate the relative path needed to access files on the first argument
-    relative from the second argument.
+    examplesFromAssertions :: Expr Void Import -> [Expr Void Import]


I wonder whether this can handle properties like https://github.com/dhall-lang/dhall-lang/blob/f035ea1947d9392bcedbe7f8c563f33559370efd/Prelude/Natural/lessThanEqual#L14-L16 or this example.

Admittedly I'm also unsure what the expected output should look like. ;)

mmmm maybe we could traverse the Lam constructor on the binding and remove the assert there?

The issue is that we might need to traverse all the binded values in the file and check if there is at least an assert, which might add some overhead.

If a binding has assertions, we remove them. Then for that property example:

let property0 = λ(n : Natural) → assert : lessThanEqual 0 n ≡ True

we should strip the assert and we might be ready to go:

λ(n : Natural) → lessThanEqual 0 n ≡ True

dhall-docs/src/Dhall/Docs/Core.hs

sjakobi · 2020-07-07T12:16:50Z

dhall-docs/src/Dhall/Docs/Core.hs

-
-- | Generate all of the docs for a package
+createIndexes :: Text -> [DhallFile] -> [(Path Rel File, Text)]
+createIndexes packageName files = map toIndex dirToDirsAndFilesMapAssocs


The code inside this function feels kind of messy and impenetrable. I'm not sure how to best fix it though.

I somewhat wonder whether the (Map (Path Rel Dir)) is a good fit for this logic, or if possibly a custom tree structure would be a better fit.

IKR!?

The most messy part IMO is the dirToDirsMap, that was the best I could do to get it right.

If you're wondering what it does (probably could guide me to another solution) is to try to compact listed paths as much as it can.

for isntance, the folders at the tasty/data/package are the following:

./ ./a/ ./a/b/ ./a/b/c/ ./deep/ ./deep/nested/ ./deep/nested/folder

but on ./deep/ and ./deep/nested/ folders there is no file.

So it's kind of dissapointing if the package directory at ./ listed ./deep/index.html and the last listed ./deep/nested/index.html to finally list ./deep/nested/folder/index.html.

So what it does is to compact paths as much as it cans, without altering the actual directory structure.

An expected output for that directory structure is the output in the golden files:

./ ./a/ ./a/b/ ./a/b/c/ ./deep/nested/folder

I'm still thinking a better solution or at least a more readable one. Get back to you if I find out something.

@sjakobi: The most I could do was to use foldl instead of passing the whole list to go at dirToDirsMap.

Another idea I got was the following (pseudocode):

def f(sortedDirs): stack = [sortedDirs[0]] map = { sortedDirs[0] = [ ] } for i in range(sortedDirs): t = stack.top() d = sortedDirs[i] if t prefix of d: stack.push(d) map[t].add(d) else: while stack.top() not prefix of d: stack.pop() map[stack.top()].add(d) stack.push(d) return map

... but couldn't write a legible implementation, at least not better than our current one.

I haven't tried yet using a tree-like structure, but I think for that in another PR.

dhall-docs/tasty/data/package/ShouldBeIgnored.dhall

german1608 · 2020-07-07T21:41:45Z

Definetly it's an issue with index.html files. I added a test file at tasty/data/index.html with its glob pattern in dhall-docs.cabal and it didn't detect but if I rename index.html to anything else it works.

Strange! Have you worked out where the problem with the index.html arises? Is it a problem with Cabal or does it come from the Nix toolchain?

It was in our nix setup:

dhall-haskell/nix/sdist.nix

Lines 6 to 16 in 282be96

    
           predicate = path: type: 
        
             let 
        
               base = baseNameOf path; 
        
             in 
        
                !( lib.hasSuffix ".nix" base 
        
                || base == "dist" 
        
                || base == "result" 
        
                || base == ".git" 
        
                || base == "index.html" 
        
                );

It's there for a reason, so I'll keep the index-gen.html setup as it currently is.

This reverts commit f2793d0.

german1608 · 2020-07-07T22:41:06Z

I updated tasty-silver to 3.1.15 because on later versions it didn't properly showed diff between text.

It shows the diff if git diff is availble, otherwise it just pastes the golden value and the actual value

german1608 · 2020-07-07T23:04:55Z

I updated tasty-silver to 3.1.15 because on later versions it didn't properly showed diff between text.

It shows the diff if git diff is availble, otherwise it just pastes the golden value and the actual value

I don't know if this is because this update, but I found a bug in tasty-silver. I reported it on phile314/tasty-silver#24

sjakobi

Any idea why the index.html is filtered out in the sdist.nix?

Some of my comments are still open, but they don't really need to be addressed within this PR.

👍

german1608 · 2020-07-07T23:59:33Z

Any idea why the index.html is filtered out in the sdist.nix?

Actually no, maybe @Gabriel439 knows why

Some of my comments are still open, but they don't really need to be addressed within this PR.

I'll think a little about how can we improve createIndexes. I'll merge the PR if I don't get anything useful.

Regarding the properties rendering, I think that it will be worth to discuss on our weekly call.

To end, thanks for your approval @sjakobi !

Gabriella439 · 2020-07-08T01:16:09Z

@german1608: I think you can delete the line filtering out index.html. I don't remember why I added it and I don't believe it is necessary

sjakobi · 2020-07-08T12:10:18Z

dhall-docs/src/Dhall/Docs/Core.hs

+        documentation to get more information.
+    -}
+    dirToDirsMap :: Map (Path Rel Dir) [Path Rel Dir]
+    dirToDirsMap = Map.map removeHereDir $ foldl go initialMap dirs


In general, I'd recommend using foldl' over foldl for folding over lists. There's a bit of a discussion here: hasura/graphql-engine#2933 (comment)

german1608 added the dhall-docs label Jul 6, 2020

feat(dhall-docs): Add test-setup and assertions as examples

fb9f6a6

german1608 force-pushed the debt/test-setup-and-assertions branch from ae6aa0d to fb9f6a6 Compare July 6, 2020 15:13

Gabriella439 reviewed Jul 6, 2020

View reviewed changes

german1608 added 4 commits July 6, 2020 12:05

remove commented code and improve documentation

957d040

make GeneratedDocs a MonadWriter

20fd5a5

improve error messages

c3b43dd

improve fileAnIssue error message

87091c0

german1608 added 2 commits July 6, 2020 16:53

try commenting patterns

1dea759

fix tests

f47a362

add again golden files to extra-source-files

c1830d9

pretty print html before test output

db0bff5

add missing .nix files

7f778b0

index.html files renamed to index-gen.html on tests

6fd324b

sjakobi reviewed Jul 7, 2020

View reviewed changes

german1608 added 4 commits July 7, 2020 12:18

add inline to instance * GeneratedDocs

f2793d0

use partitionEithers instead of separate lefts and rights

176778c

remove commented code

6d8c821

fix typo

1809078

german1608 added 2 commits July 7, 2020 17:41

Revert "add inline to instance * GeneratedDocs"

15fd07a

This reverts commit f2793d0.

use tasty-silver 3.1.15 for better diff support

9bf30e2

sort on path after getting all dhall files

2598680

sjakobi approved these changes Jul 7, 2020

View reviewed changes

german1608 added 3 commits July 7, 2020 23:51

remove index.html from sdist.nix filter

b54d81e

improve documentation

ef89376

use foldl

8a1b7c6

german1608 added the merge me label Jul 8, 2020

mergify bot merged commit 0008cd0 into master Jul 8, 2020

mergify bot deleted the debt/test-setup-and-assertions branch July 8, 2020 05:01

sjakobi reviewed Jul 8, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dhall-docs): Add test-setup and assertions as examples #1899

feat(dhall-docs): Add test-setup and assertions as examples #1899

german1608 commented Jul 6, 2020

sjakobi commented Jul 6, 2020

german1608 commented Jul 6, 2020

Gabriella439 commented Jul 6, 2020

german1608 commented Jul 6, 2020

german1608 commented Jul 6, 2020

sjakobi commented Jul 6, 2020

german1608 commented Jul 6, 2020

german1608 commented Jul 6, 2020

german1608 commented Jul 6, 2020 •

edited

Loading

sjakobi commented Jul 6, 2020

sjakobi commented Jul 6, 2020

german1608 commented Jul 6, 2020

german1608 commented Jul 6, 2020

german1608 commented Jul 6, 2020 •

edited

Loading

german1608 commented Jul 7, 2020

german1608 commented Jul 7, 2020

sjakobi commented Jul 7, 2020

sjakobi Jul 7, 2020

german1608 Jul 7, 2020

sjakobi Jul 7, 2020

german1608 Jul 7, 2020

german1608 Jul 8, 2020

german1608 commented Jul 7, 2020

german1608 commented Jul 7, 2020

german1608 commented Jul 7, 2020

sjakobi left a comment

german1608 commented Jul 7, 2020 •

edited

Loading

Gabriella439 commented Jul 8, 2020

sjakobi Jul 8, 2020

feat(dhall-docs): Add test-setup and assertions as examples #1899

feat(dhall-docs): Add test-setup and assertions as examples #1899

Conversation

german1608 commented Jul 6, 2020

sjakobi commented Jul 6, 2020

german1608 commented Jul 6, 2020

Gabriella439 commented Jul 6, 2020

german1608 commented Jul 6, 2020

german1608 commented Jul 6, 2020

sjakobi commented Jul 6, 2020

german1608 commented Jul 6, 2020

german1608 commented Jul 6, 2020

german1608 commented Jul 6, 2020 • edited Loading

sjakobi commented Jul 6, 2020

sjakobi commented Jul 6, 2020

german1608 commented Jul 6, 2020

german1608 commented Jul 6, 2020

german1608 commented Jul 6, 2020 • edited Loading

german1608 commented Jul 7, 2020

german1608 commented Jul 7, 2020

sjakobi commented Jul 7, 2020

sjakobi Jul 7, 2020

Choose a reason for hiding this comment

german1608 Jul 7, 2020

Choose a reason for hiding this comment

sjakobi Jul 7, 2020

Choose a reason for hiding this comment

german1608 Jul 7, 2020

Choose a reason for hiding this comment

german1608 Jul 8, 2020

Choose a reason for hiding this comment

german1608 commented Jul 7, 2020

german1608 commented Jul 7, 2020

german1608 commented Jul 7, 2020

sjakobi left a comment

Choose a reason for hiding this comment

german1608 commented Jul 7, 2020 • edited Loading

Gabriella439 commented Jul 8, 2020

sjakobi Jul 8, 2020

Choose a reason for hiding this comment

german1608 commented Jul 6, 2020 •

edited

Loading

german1608 commented Jul 6, 2020 •

edited

Loading

german1608 commented Jul 7, 2020 •

edited

Loading