-
Notifications
You must be signed in to change notification settings - Fork 194
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
[glsl-in] consolidate tests #206
Conversation
pjoe
commented
Sep 18, 2020
- remove simple lex test as was covered by others
- combine related tests to single function
- remove redundant prefix in test names
- remove simple lex test as was covered by others - combine related tests to single function - remove redundant prefix in test names
Perhaps, you could refactor this some more with the ideas from 70aa8b7 ? |
@kvark though that does make the test code a bit cleaner, I don't like the way failures are then reported at the line number in the |
Could we use rust-lang/rust#72445 ? |
Not really sure I understand, how this would change things? |
If we put |
Ok, so the remaining question is more about asserts in loops vs. pointing to individual line. Keeping them individual:
|
Seems fine to me. Are you concerned about this look? |
No, this is the way it looks with individual asserts (like currently). In terms of how failures are reported, this is how I'd prefer. I'll try adding similar screenshot tomorrow for the other solution with a helper function, looping over expected tokens, and returning a |
Just to clarify, reducing the number of |
Right, as it stands the PR reduces number of lexer test functions to 4, would you prefer it all the way down to 1? The matter of number of asserts relates to using a common helper function to check against an array of expected tokens, instead of asserting them individually. |
As agreed on matrix, consolidated lex tests to a single function |