test: new test runner changes from lb support #2901
Conversation
dariakp
left a comment
dariakp
left a comment
There was a problem hiding this comment.
Just a few outstanding questions/concerns here
| // TODO: NodeJS Driver has extra events | ||
| // expect(actual).to.have.lengthOf(expected.length); | ||
| if (actual.length !== expected.length) { | ||
| // const actualNames = actual.map(a => a.constructor.name); |
There was a problem hiding this comment.
this seems to be an empty code block
There was a problem hiding this comment.
Relevant convo over on the bigger PR: #2742 (comment)
There was a problem hiding this comment.
@nbbeeken Thanks! still think that we should maybe remove the if block and the code that attempts to do things and add a comment in the code summarizing this:
It was intentional as the Node driver does not execute a killCursors after a getMore has errored, which causes the absence of the killCursors events in some cases. I could not find anywhere so far in the specs that states an errored getMore requires an immediate killCursors afterwards, and all the driver does in this case is cleanup the cursor (but not explicitly close() it).
(maybe even file a ticket to explore this further, since if we know exactly what's out of the ordinary, we may be able to filter explicitly those cases out)
There was a problem hiding this comment.
This check is now back and asserting the event lengths, so no comment needed.
There was a problem hiding this comment.
It looks like some of the new snapshot tests are failing now with that check, I don't think you need to take care of it here because I need to sort out those tests anyway, but if you could add them to the skip list, that would allow the pipeline to stay green on main
There was a problem hiding this comment.
snapshot-sessions-unsupported-ops
- Server returns an error on findOneAndUpdate with snapshot
- Server returns an error on deleteOne with snapshot
- Server returns an error on updateOne with snapshot
There was a problem hiding this comment.
I've skipped the one failing test now.
dariakp
added
the
Primary Review
dariakp
added
Team Review
* test: new test runner changes from lb support * test: refactoring unified runner * test: bring back unified event length check * test: run txn tests in manual lb tests * test: skip failing snapshot test and refactor matchers * test: remove lb manual test * test: skip other failed tests
No description provided.