-
Notifications
You must be signed in to change notification settings - Fork 41
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
RFC: Revamp TestUtils & tests #14
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks clean to me (although I have zero authority here...)
@mattdamon108 @cristianoc What about this PR? The previously existing tests were removed in #49. Are we interested in re-adding tests? |
Not sure. Do they add value to justify the maintenance? E.g w.r.t. snapshot test where one just looks at the generated code? Could go either way. No opinion. |
I have no opinion. Actually, not sure what we should test for the binding module. |
Checking in che compiled output seems by far the easiest thing. And tells everything there is to know. |
@@ -16,7 +16,7 @@ jobs: | |||
|
|||
strategy: | |||
matrix: | |||
node-version: [10.x, 12.x] | |||
node-version: [14.x] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably add 16 since it's the current LTS release.
→ Removes Jest
→ Updates peerDependencies
Test files render
Setup
Assertions setup
Tests
Test output render