-
Notifications
You must be signed in to change notification settings - Fork 1.8k
test(NODE-5898): small fixes and unskips of test TODOs #4055
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-5898): small fixes and unskips of test TODOs #4055
Conversation
test/integration/client-side-encryption/client_side_encryption.prose.test.js
Show resolved
Hide resolved
| ({ description }) => { | ||
| // This is skipped because our command monitoring happens at the connection | ||
| // level and is using the server reply for the single insert that the bulk | ||
| // performed since it was only one document in the test. The test expectation | ||
| // is that we are using the bulk write result which was returned to the user | ||
| // as the reply in the command succeeded event instead of our raw reply from | ||
| // the server. There's nothing we can change here. | ||
| return description.includes( | ||
| 'A successful unordered bulk write with an unacknowledged write concern' | ||
| ) | ||
| ? `Test not applicable to Node.` | ||
| : false; | ||
| } |
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.
just added a more applicable skip reason.
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 do not fully understand why this doesn't apply to us. In a world where we were writing the driver from scratch could it obey this test? if so, could this one day, allegedly, be addressed?
This reverts commit da3fe14.
Description
What is changing?
While auditing the existing skipped tests, I found a couple of quick fixes. This PR unskips some tests that not longer fail, removes some unnecessary skip reason assignment, and removes some unnecessary TODO comments.
Is there new documentation needed for these changes?
What is the motivation for this change?
Double check the following
npm run check:lintscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript