-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
test(NODE-5342): modernize explain tests #3720
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.
Overall, great job so far!
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.
I did not review the entire set of tests, but I think it would be worthwhile to double check all test cases and make sure that 1) the "it" blocks accurately capture the behavior of interest and 2) the assertions fully test the described behavior. I called out one example of what I mean, let me know if you have any questions.
Concerns addressed in subsequent commits
Description
What is changing?
explain tests modernized
removed metadata
converted to typescript
Is there new documentation needed for these changes?
no
What is the motivation for this change?
new things are nice and look clean
Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript