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

Test syntax error messages #1934

Merged
merged 3 commits into from Jul 27, 2018

Conversation

Projects
None yet
3 participants
@trefis
Copy link
Contributor

commented Jul 26, 2018

In preparation for a potential switch to menhir (but simply good to have regardless), this PR adds a test case for (almost[1]) every production in the grammar ending in an error token.

Some notes:

  • it is not enough to write module M = struct to get:
    File "unclosed_struct.ml", line 10, characters 0-0:
    Error: Syntax error: 'end' expected
    File "unclosed_struct.ml", line 8, characters 11-17:
    Error: This 'struct' might be unmatched
    
    One has to go the extra distance and write module M = struct type t = T. I'm guessing that's because otherwise some other error rule gets in the way.
  • I failed to produce the expected error message in a few cases.
  • to properly test errors at the structure / signature level, I created a test file per grammar production which is compiled with ocamlc. For expressions and patterns it's fine to put several in one file and call the toplevel, so that's what I did.

[1]: I believe the only missing rules are the redundant ones between pattern and pattern_no_exn. Everything else should be covered.

@trefis trefis requested a review from gasche Jul 26, 2018

@trefis trefis force-pushed the trefis:test-syntax-errors branch from c5336a1 to 8dddbb0 Jul 27, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

We have already discussed this privately, but I find this very nice and am impressed with how far you were able to go.

For the * toplevel tests, is there a particular reason to not use expect tests?

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

(I'll add this GPR number to the Menhir-parser Changes entry.)

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2018

For the * toplevel tests, is there a particular reason to not use expect tests?

My initial attempt was using expect tests, but the test failed on the first syntax error, which was printed on stderr.
It's possible (even likely) that that was when trying structures, and that it would work fine for expressions and patterns (which is where I am currently using toplevel tests), but I didn't investigate.

I usually have a strong dislike of toplevel tests, because it's really annoying to link the output to the input. But here the test files are small enough that it is not (yet) a problem.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2018

Apparently travis refuses to acknowledge the "no-change-entry-needed" label. But apart from that things seem to be fine.

@gasche: do you want to approve?

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

I don't understand, are you suggesting that syntax errors for some syntactic categories (but not others) are printed on stderr, while typing errors (which we use expect-tests for) are printed on stdout? Isn't this an inconsistency that we could/should fix?

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2018

I don't understand, are you suggesting that syntax errors for some syntactic categories (but not others) are printed on stderr, while typing errors (which we use expect-tests for) are printed on stdout? Isn't this an inconsistency that we could/should fix?

Not exactly.
expect_test captures both stdout and stderr when executing (which means typing + running) a phrase.
But it doesn't capture anything during parsing (I just checked).

To me this make sense, and I don't know how you could fix it. Let's assume you've captured an error message when you've failed to parse the file: where do you put that message? You don't know where the expect block is, there might not even be one.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2018

@gasche I don't know which mecanism the testing framework uses but note that whenever ocaml is used in script mode, it may report errors on stdout which is annoying see MPR7202.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

I see. I think we could fix the issue, but it would require non-trivial changes in testsuite/tools/expect_test.ml. (Currently it first parses all phrases, and then runs then and collect the outputs, we could start output collection during the parsing phase already by using repeated calls to Parse.toplevel_phrase instead of one call to Parse.use_file.)

(For the menhir-parsing branch) Expect tests currently only use the yacc parser, we should change this to have them exercise the menhir as well.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

@dbuenzli noted, but I can't work on this right now. Is there a reason why you are reluctant to provide a patch yourself? (In some ways you have pushed toplevel scripts farther along than others, so it would make sense for you to feel confident in your ability to improve the toplevel logic/codebase for your needs.)

@gasche

gasche approved these changes Jul 27, 2018

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2018

why you are reluctant to provide a patch yourself?

I'm not reluctant. My time is limited.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

That makes perfect sense, thanks. I just wanted to make sure there wasn't another barrier to contribution.

@trefis trefis merged commit d8dc9f3 into ocaml:trunk Jul 27, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@trefis trefis deleted the trefis:test-syntax-errors branch Jul 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.