Skip to content

Fix test for random initialized node (#4134) #4144

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

Merged
merged 3 commits into from
Oct 21, 2019
Merged

Conversation

russcam
Copy link
Contributor

@russcam russcam commented Oct 15, 2019

The test was using a local method that yielded a single pool while later .Take(50)
was requested on this sequence. This always yields an enumeration with 1 element
which increases the chances for the assertion to turn false.

Rather than relying on Thread.Sleep to create different seeded randoms the StaticConnectionPool
now defines a protected constructor that allows one to pass in a seed value.

(cherry picked from commit 9734f04)

The test was using a local method that yielded a single pool while later .Take(50)
was requested on this sequence. This always yields an enumeration with 1 element
which increases the chances for the assertion to turn false.

Rather than relying on Thread.Sleep to create different seeded randoms the StaticConnectionPool
now defines a protected constructor that allows one to pass in a seed value.

(cherry picked from commit 9734f04)
@russcam
Copy link
Contributor Author

russcam commented Oct 15, 2019

port to master of #4134

Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

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

I think two properties gained public setters again inadvertently other than that LGTM, thanks for doing the forward port!

@@ -62,7 +77,7 @@ protected StaticConnectionPool(IEnumerable<Node> nodes, Func<Node, float> nodeSc
public virtual bool SupportsReseeding => false;

/// <inheritdoc />
public bool UsingSsl { get; }
public bool UsingSsl { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public bool UsingSsl { get; set; }
public bool UsingSsl { get; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be a private setter

@@ -75,10 +90,10 @@ protected List<Node> AliveNodes
}
}

protected IDateTimeProvider DateTimeProvider { get; }
protected IDateTimeProvider DateTimeProvider { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
protected IDateTimeProvider DateTimeProvider { get; set; }
protected IDateTimeProvider DateTimeProvider { get; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be a private setter

@russcam
Copy link
Contributor Author

russcam commented Oct 21, 2019

Would you mind taking a look again @Mpdreamz

@Mpdreamz
Copy link
Member

@russcam compilation failed on CI

Fat-fingered a 'B' when running a build with CTRL + SHIFT + b...
@russcam
Copy link
Contributor Author

russcam commented Oct 21, 2019

D'oh, fat fingered a B into source when running a build. Fixed now. Let this be a warning to future self when attempting to code in between conference sessions 🤦‍♂

@russcam
Copy link
Contributor Author

russcam commented Oct 21, 2019

Merging this in; failing tests are not related to this change,

@russcam russcam merged commit d0f99d4 into master Oct 21, 2019
@Mpdreamz Mpdreamz deleted the fix/randomize-nodes branch November 6, 2019 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants