-
Notifications
You must be signed in to change notification settings - Fork 43
RFC: Revamp TestUtils & tests #14
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
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