Skip to content

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!


/// <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

}

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.

3 participants