Skip to content

test(NODE-3188): refactor transaction spec tests: #2831

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 17, 2021
Merged

test(NODE-3188): refactor transaction spec tests: #2831

merged 3 commits into from
Jun 17, 2021

Conversation

durran
Copy link
Member

@durran durran commented Jun 7, 2021 •
edited
Loading

  • Adds new unified tests for mongos unpinning.
  • Refactors legacy tests into legacy directory.
  • Also addresses NODE-3228 and NODE-3216 by syncing all the related txn spec tests.
  • Fixes bug in the unified test runner asserting incorrect sessions on cursor operations.

@durran durran force-pushed the NODE-3188 branch 3 times, most recently from e0ed6fc to 797c698 Compare June 8, 2021 14:22
durran
durran commented Jun 8, 2021
View reviewed changes
@durran durran force-pushed the NODE-3188 branch 4 times, most recently from 621db4a to 5859889 Compare June 8, 2021 16:58
durran
durran commented Jun 8, 2021
View reviewed changes
durran
durran commented Jun 8, 2021
View reviewed changes
@durran durran force-pushed the NODE-3188 branch 20 times, most recently from 9552db1 to 001b2a1 Compare June 10, 2021 14:43
durran
durran commented Jun 10, 2021
View reviewed changes
@durran durran force-pushed the NODE-3188 branch 2 times, most recently from affa674 to 2a504b1 Compare June 10, 2021 22:37
@durran durran requested a review from emadum June 10, 2021 22:37
@durran
test(NODE-3188): refactor transaction spec tests:
01d7b35 
- Adds new unified tests for mongos unpinning.
- Refactors legacy tests into legacy directory.
emadum
emadum approved these changes Jun 11, 2021
View reviewed changes
Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

@emadum emadum requested a review from nbbeeken June 11, 2021 17:43
@durran durran mentioned this pull request Jun 12, 2021
Merged
@durran durran added the Team Review Needs review from team label Jun 15, 2021
@durran durran requested a review from dariakp June 15, 2021 14:25
dariakp
dariakp reviewed Jun 15, 2021
View reviewed changes
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one suggestion for clean up, otherwise LGTM

nbbeeken
nbbeeken suggested changes Jun 15, 2021
View reviewed changes
@dariakp dariakp requested a review from nbbeeken June 15, 2021 18:37
dariakp
dariakp requested changes Jun 15, 2021
View reviewed changes
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidentally reset Neal's review, so marking request changes on his behalf

nbbeeken
nbbeeken approved these changes Jun 15, 2021
View reviewed changes
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@durran durran force-pushed the NODE-3188 branch 2 times, most recently from e470de6 to 98e19c3 Compare June 15, 2021 19:53
@dariakp
Copy link
Contributor

dariakp commented Jun 15, 2021

@durran looks like that test change made a lot of things fail due to session being undefined (also not sure if you saw my comment above regarding recommended clean up)

@durran durran force-pushed the NODE-3188 branch 2 times, most recently from f679bc9 to 2b9930d Compare June 15, 2021 20:32
@durran
Copy link
Member Author

durran commented Jun 15, 2021 •
edited
Loading

@durran looks like that test change made a lot of things fail due to session being undefined (also not sure if you saw my comment above regarding recommended clean up)

@dariakp yes I had a Slack conversation with @nbbeeken and just pushed a change to resolve that. I also have your suggestion lined up in another commit that's incoming after the next test run passes.

For the session undefined issues, a cursor can be instantiated without a session, which would cause Object.getOwnPropertySymbols() to never return the symbol in that case, causing the session lookup to never resolve in the unified tests. So I brought back the setter on the cursor, but made both the getter and setter for session internal, since it doesn't really make sense that either would be exposed to the user. The documented API to get handles on sessions is via the MongoClient and not the cursor.

dariakp
dariakp approved these changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@emadum emadum merged commit b3c3721 into 4.0 Jun 17, 2021
@emadum emadum deleted the NODE-3188 branch June 17, 2021 15:49
ljhaywar pushed a commit that referenced this pull request Nov 9, 2021
@durran @ljhaywar
test(NODE-3188): refactor transaction spec tests: (#2831)
f951a3b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emadum emadum emadum approved these changes

@nbbeeken nbbeeken nbbeeken approved these changes

@dariakp dariakp dariakp approved these changes

Assignees

@emadum emadum

Labels
Team Review Needs review from team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@durran @dariakp @emadum @nbbeeken