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.

Sign up for GitHub

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

Jump to bottom

fix #190: refactor parser tests into separate modules #194

Merged
merged 2 commits into from
Jul 19, 2021
Merged

fix #190: refactor parser tests into separate modules #194

merged 2 commits into from
Jul 19, 2021

Conversation

ulmer-a
Copy link
Collaborator

@ulmer-a ulmer-a commented Jul 16, 2021 •
edited
Loading

  • parser/tests/parser_test.rs split up into multiple separate modules in parser/tests as well as parser/tests/parse_errors
  • meaningful names for the files and readable file length

@ghaith
Copy link
Collaborator

ghaith commented Jul 16, 2021

Thank you for the fix!

Could you please rebase this on the parser_recovery branch, or wait until that branch is submitted?
We made a lot of changes to the tests there to introduce the parsing.

@codecov
Copy link

codecov bot commented Jul 16, 2021 •
edited
Loading

Codecov Report

Merging #194 (5b72d00) into master (9c334e7) will increase coverage by 0.00%.
The diff coverage is 99.93%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #194   +/-   ##
=======================================
  Coverage   95.54%   95.54%           
=======================================
  Files          47       55    +8     
  Lines       21037    20997   -40     
=======================================
- Hits        20099    20062   -37     
+ Misses        938      935    -3
Impacted Files Coverage Δ
src/parser/tests.rs 100.00% <ø> (ø)
...tests/parse_errors/parse_error_statements_tests.rs 100.00% <ø> (ø)
src/parser/tests/initializer_parser_tests.rs 99.73% <99.73%> (ø)
src/parser/tests/container_parser_tests.rs 100.00% <100.00%> (ø)
src/parser/tests/function_parser_tests.rs 100.00% <100.00%> (ø)
src/parser/tests/misc_parser_tests.rs 100.00% <100.00%> (ø)
...tests/parse_errors/parse_error_containers_tests.rs 89.41% <100.00%> (+1.30%) ⬆️
...er/tests/parse_errors/parse_error_messages_test.rs 100.00% <100.00%> (ø)
src/parser/tests/program_parser_tests.rs 100.00% <100.00%> (ø)
src/parser/tests/statement_parser_tests.rs 100.00% <100.00%> (ø)
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c334e7...5b72d00. Read the comment docs.

@riederm
Copy link
Collaborator

riederm commented Jul 17, 2021

cool thx, looks really good

there was a bit of an unlucky timing, i had a pretty big commit pending for quite some time ... I just looked at the conflicts locally and its really not that bad.

you need to just apply the (real) changes form parser_tests.rs to where you moved the test. Its not that bad, it looks more intimidating than it is.

@ulmer-a ulmer-a force-pushed the refactor_parser_tests branch from 77550fa to c57afa8 Compare July 19, 2021 07:34
ghaith
ghaith requested changes Jul 19, 2021
View reviewed changes
Copy link
Collaborator

@ghaith ghaith 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, but I noticed that some of the tests are not in the correct place. I think for the most part you would be able to move the entire module one directory up, but for some, you might still have to split them into modules.

src/parser/tests/parse_errors/parse_error_function_tests.rs Outdated
use pretty_assertions::*;

#[test]
fn simple_foo_function_can_be_parsed() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

None of the tests in this file are error tests, I think this module can move below the parser module instead of the parse_errors one

src/parser/tests/parse_errors/parse_error_initializer_tests.rs Outdated
use crate::parser::{parse, tests::lex};

#[test]
fn initial_scalar_values_can_be_parsed() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same issue here, these are not error tests

src/parser/tests/parse_errors/parse_error_messages_test.rs Outdated
msg
);
} else {
panic!("Expected parse error but didn't get one.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you try : assert_eq!(Err(Diagnostic...,parse_result)? Maybe that removes the coverage warning.

src/parser/tests/parse_errors/parse_error_program_tests.rs Outdated
};

#[test]
fn simple_foo_program_can_be_parsed() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is also no error test

src/parser/tests/parse_errors/parse_error_statements_tests.rs Outdated
@@ -970,3 +970,281 @@ fn test_case_without_condition() {
),]
);
}

#[test]
fn empty_statements_are_are_parsed() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some of these tests are not error related either

src/parser/tests/parse_errors/parse_error_type_tests.rs Outdated
use pretty_assertions::*;

#[test]
fn simple_struct_type_can_be_parsed() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Non error tests

src/parser/tests/parse_errors/parse_error_variable_tests.rs Outdated
}

#[test]
fn two_global_vars_can_be_parsed() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Non error tests

@ghaith ghaith linked an issue Jul 19, 2021 that may be closed by this pull request
Refactor src/parser/tests/parser_tests.rs into seperate modules #190
Closed
@ulmer-a
Copy link
Collaborator Author

ulmer-a commented Jul 19, 2021

Looks good, but I noticed that some of the tests are not in the correct place. I think for the most part you would be able to move the entire module one directory up, but for some, you might still have to split them into modules.

Thanks for the comments, this should now be resolved.

@ghaith ghaith merged commit d796559 into PLC-lang:master Jul 19, 2021
@ulmer-a ulmer-a deleted the refactor_parser_tests branch July 19, 2021 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ghaith ghaith ghaith approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor src/parser/tests/parser_tests.rs into seperate modules
3 participants
@ulmer-a @ghaith @riederm