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 snapshots test back to parser #82

Closed
psteinroe opened this issue Dec 17, 2023 · 6 comments
Closed

add snapshots test back to parser #82

psteinroe opened this issue Dec 17, 2023 · 6 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@psteinroe
Copy link
Collaborator

psteinroe commented Dec 17, 2023

Planned Work

We already had it in place but dropped it because it's unnecessary now that the parser panics on invalid statements. but turns out it's actually quite useful when reviewing pull requests for #51. we should add it back using insta as before. we should be able to simply copy it from the git history of the statement_parser_test file.

@psteinroe psteinroe added enhancement New feature or request good first issue Good for newcomers labels Dec 17, 2023
@cvng
Copy link
Contributor

cvng commented Dec 17, 2023

Yeah, I was exactly thinking about this, having insta back will help making PRs related to #51 less tedious. Another great addition could be testing against postgres regression tests as well

@psteinroe
Copy link
Collaborator Author

Another great addition could be testing against postgres regression tests as well

ohh thats a great idea! we can just copy the sql files, or add the entire postgres repo as a submodule to keep it up-to-date. wdyt?

@cvng
Copy link
Contributor

cvng commented Dec 18, 2023

I was planning at first to pull the sql files with a tiny script and later add it as a submodule when progress has been made

@psteinroe
Copy link
Collaborator Author

@cvng can you open a separate issue for pulling in the regression tests?

@cvng
Copy link
Contributor

cvng commented Dec 18, 2023

@psteinroe sure, I already had a few PRs stacked for #51, so I will proceed to pull the postgres tests once those are merged

EDIT: also, should we somehow consolidate parser e2e & unit tests? eg. add get_node_properties in the snapshots

@psteinroe
Copy link
Collaborator Author

EDIT: also, should we somehow consolidate parser e2e & unit tests? eg. add get_node_properties in the snapshots

the get_node_properties tests should rather serve as a help for devs to figure out the right props fast - I would keep them separated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants