Skip to content
This repository has been archived by the owner on Feb 4, 2022. It is now read-only.

fix(secondaries): fixes connection with secondary readPreference #258

Merged
merged 1 commit into from
Dec 19, 2017

Conversation

daprahamian
Copy link
Contributor

Ensures that when readPreference is secondary, the connect
event is not triggered until connection has been established
with a secondary.

Fixes NODE-1089

Copy link
Member

@mbroadst mbroadst left a comment

Choose a reason for hiding this comment

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

Just one comment, great job

@@ -1337,4 +1337,104 @@ describe('ReplSet Read Preferences (mocks)', function() {
});
}
});

Copy link
Member

Choose a reason for hiding this comment

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

Can we please move this test to tests/unit, and use the ReplSetFixture to cut out a lot of the code dupe, and allow us to focus on the actual test conditions.

@daprahamian daprahamian force-pushed the NODE-1089 branch 2 times, most recently from dc3b225 to 0f38d9e Compare December 15, 2017 17:24
Ensures that when readPreference is `secondary`, the `connect`
event is not triggered until connection has been established
with a secondary.

Fixes NODE-1089

replSet.on('connect', server => {
let err;
try {
Copy link
Member

Choose a reason for hiding this comment

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

do we actually need this try/catch? if the assertion fails we will still get the message during the test run (e.g. I think there is already a try/catch internal to mocha that provides this same behavior)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since it is asynchronous, it won't work

@daprahamian daprahamian merged commit 0060ad7 into 3.0.0 Dec 19, 2017
@daprahamian daprahamian deleted the NODE-1089 branch December 19, 2017 16:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants