Write unit tests for parser

[VMatthijs]

We currently have a lot of integration tests, but unit tests are missing.

[bob-carpenter]

It would be good to coordinate with @seantalts about the amount of unit testing this will require and for which components. The old system never really unit tested the generator in any significant way and the AST tests were huge but trivial. Mostly it was testing end-to-end results on parsing grammar fragments to make sure they either parsed (at all, usually not with generated code checks) or generated the correct error messages.

[VMatthijs]

Yes. If you guys have thoughts on what precisely should be unit tested, I would love to know.

I am currently planning to add some expect tests to make sure that:

1. if you strip the decorations of the AST after semantic checking, you end up with the original AST (very minimal test for semantic_check)
2. if you parse, pretty print and parse again, that's the same as parsing once (pretty decent test for correctness of the pretty printer, provided that the parser is correct).

What I really don't know is what unit tests to write for the parser. I feel like the semantic check is tested pretty well by the end-to-end tests. The parser, I am less sure about, until we start generating code and testing that. The AST does not need any tests, I would think.

[seantalts]

I'm not sure that's a useful test for the type checker - is that an invariant we care about?

I think the way to think about these more granular tests is that anywhere where a function is doing something pretty tricksy or unobvious, you can add a test that demonstrates its behavior in that circumstance. For me, this often occurs while writing the functions because I actually need to test what I'm writing as I'm writing it. Then, you can just leave the test in.

[VMatthijs]

I'm not sure that's a useful test for the type checker - is that an invariant we care about?

I think it is an invariant we care about. The semantic check should not change the AST. (Otherwise something is quite clearly wrong.) It should only decorate it or maybe throw. I agree it's a pretty low bar and by no means shows correctness, but it's a good sanity check.

I think the way to think about these more granular tests is that anywhere where a function is doing something pretty tricksy or unobvious, you can add a test that demonstrates its behavior in that circumstance. For me, this often occurs while writing the functions because I actually need to test what I'm writing as I'm writing it. Then, you can just leave the test in.

Thought about that way, I think the only part of the parser that was tricky to get right was the precedence of the operators and the dangling else clauses. So should I write expect tests for those?

For semantic checking, I think the integrations tests have good coverage. Whenever there was something tricky, I've added test models if there weren't any yet.

Does anything else come to mind?

[seantalts]

I think it is an invariant we care about. The semantic check should not change the AST. (Otherwise something is quite clearly wrong.) It should only decorate it or maybe throw. I agree it's a pretty low bar and by no means shows correctness, but it's a good sanity check.

I think this would be an interesting use case to try out Jane Street's quickcheck testing library, since you basically just need to fuzz input and check a simple invariant.

Thought about that way, I think the only part of the parser that was tricky to get right was the precedence of the operators and the dangling else clauses. So should I write expect tests for those?

That sounds good. I can then add some more for anything else I find confusing once the testable hooks are there (e.g. additional Parser.Interpreted.<start symbol> functions are available).

I think you're generally right about our end-to-end test coverage being good. I am just thinking about these more granular tests as being documentation that is automatically tested to be up-to-date. So in addition to testing anything that was very tricky, you could add tests to illustrate examples and help explain the code.

[VMatthijs]

Thanks, @seantalts ! Will do.

[VMatthijs]

I tested that the pretty printer is a one-sided inverse to the parser. Moreover, the integration tests are tracking changes to the expected pretty printed code. This expected output is now probably correct.

Also, I just added unit tests for the parser to check that it does operator precedence correctly.

[VMatthijs]

@seantalts , I've added a bunch of unit tests for the parser. Could you have a look if they suffice?

On a different note, both @bob-carpenter and I felt like it might be cleaner to move the unit tests to their own file.

[seantalts]

Sure thing! I think you forgot to open a PR though. Could you do that and I'll comment on it?

[seantalts]

Also I think that sounds reasonable to have most unit tests in their own file. I also think it's worth having very short expect (and maybe quickcheck) inline to serve as documentation.

[VMatthijs]

Will open a PR moving the unit tests to their own files once my current PR is moved. (Would like to avoid merge conflicts.)

[seantalts]

I'm not sure if this solves the merge conflicts thing, but with git you can also base a branch off an existing branch: git checkout existing-branch and then just create a new branch there: git checkout -b new-branch-name. If you end up making changes to existing-branch, you can merge those in with new-branch-name normally (ie while you're on the new branch, git merge existing-branch). Conflicts should be minimal.

[VMatthijs]

Thanks!

[seantalts]

This seems like an interesting test tool: https://github.com/mirage/alcotest