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

Add test coverage to the lexer #55

Merged
merged 2 commits into from
Mar 30, 2023
Merged

Add test coverage to the lexer #55

merged 2 commits into from
Mar 30, 2023

Conversation

jansorg
Copy link
Contributor

@jansorg jansorg commented Mar 30, 2023

This PR adds test coverage to the lexer.
It adds a parameterized lexer test to automatically execute lexer tests for src/test/data/ParserTest/*.nix. I prefer this approach over keeping a long list of @Test methods in sync with the files src/test/data/ParserTest/*.nix.

I'm reusing the set of Nix files of the ParserTest instead of copying all to another directory. Please let me know if you'd like a copy instead or if I should rename ParserTest to something else.

The first commit adds the test and the needed infrastructure to make it work. The base lexer test class is relying on the test execution policy, which is now set up with the build system.
The other commit adds the reference data using the current lexer implementation.

My work is a courtesy of https://github.com/monogon-dev and a preparation for a PR to #39, which will need changes to the lexer.

Copy link
Contributor

@JojOatXGME JojOatXGME left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I am not sure if these tests were strictly necessary since ParsingTest also checks the tokens, but I am happy to merge this PR.

It adds a parameterized lexer test to automatically execute lexer tests for src/test/data/ParserTest/*.nix.

It seems that with this approach using a parameterized test, IntelliJ's UI does not support jumping from the node in the test report directly to the test data. Anyway, your approach still seems favorable.


@Override
public String getHomePath() {
return System.getProperty("plugin.testDataPath");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I am wondering if this could also replace ParsingTest.getTestDataPath() and ParsingFailTest.getTestDataPath().

@Override
protected String getTestDataPath() {
return "src/test/testData";
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should work, I think. As far as I understand this is only working because it's using the current working dir (i.e. the root project directory) as default.

@JojOatXGME JojOatXGME merged commit bb547b4 into NixOS:master Mar 30, 2023
@jansorg
Copy link
Contributor Author

jansorg commented Apr 1, 2023

Thanks for merging!
In my experience separate tests for the lexer are very helpful when you're changing the .flex file without touching the parser .bnf file. A clean list of tokens instead of a nested tree of nodes and tokens is easier to understand in such a case.

@jansorg jansorg deleted the jansorg/lexer-test-coverage branch April 1, 2023 09:35
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

Successfully merging this pull request may close these issues.

2 participants