Skip to content
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-2856): Check that defaultTransactionOptions get used from session #2845

Conversation

W-A-James
Copy link
Contributor

Description

Test verifying that transactions get their readPreference from SessionOptions passed at session instantiation

What changed?

  • lib/core/topologies/read_preference.js
  • test/functional/readpreference.test.js

@W-A-James W-A-James requested a review from emadum June 15, 2021 21:18
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.

A few small requests, but overall LGTM! 👍

@W-A-James W-A-James requested a review from emadum June 16, 2021 21:16

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@W-A-James W-A-James force-pushed the NODE-2856/3.6/ReadPerference#resolve-not-guaranteed-to-pull-from-session branch from 5d445e3 to 89dda16 Compare June 17, 2021 14:06
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 review from nbbeeken, dariakp and durran June 17, 2021 15:59
@emadum emadum added the Team Review Needs review from team label Jun 17, 2021
@W-A-James W-A-James requested a review from dariakp June 17, 2021 21:12
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.

just a small suggested improvement on the assertion

session.startTransaction();
const result = ReadPreference.resolve(client, { session: session });
expect(result).to.exist;
expect(result.mode).to.deep.equal('secondary');
Copy link
Contributor

Choose a reason for hiding this comment

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

by the way, you can use the property assertion to check the property and the value, instead of the combination of the exist and the equal checks, it usually gives a nicer message on failure, like so: expect(result).to.have.property('mode', 'secondary');

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 :)

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!

@emadum emadum merged commit 68b4665 into 3.6 Jun 22, 2021
@emadum emadum deleted the NODE-2856/3.6/ReadPerference#resolve-not-guaranteed-to-pull-from-session branch June 22, 2021 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants