- Notifications
You must be signed in to change notification settings - Fork44
RFC: Revamp TestUtils & tests#14
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
JasoonS left a comment
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. |
| 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.
Uh oh!
There was an error while loading.Please reload this page.
→ Removes Jest
→ Updates peerDependencies
Test files render
Setup
Assertions setup
Tests
Test output render