Skip to content

Conversation

soninaren
Copy link
Member

No description provided.

Copy link
Member

Choose a reason for hiding this comment

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

nit: rename to SendAsync_HostInErrorState_Returns503Immediately

Copy link
Member

@mathewc mathewc Oct 13, 2016

Choose a reason for hiding this comment

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

How can we verify that the polling loop isn't running in this case? Perhaps add a mock verification below that IsRunning was only called once? To facilitate that test, you should probably add a defaulted ctor param for hostRunningPollIntervalMs so we can configure it to a low value like 50ms for these tests so multiple polls would happen (since we're setting the max timeout to 1 second) if your check isn't working.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Currently, this doesn't test the newly introduced behavior and would pass even before the changes were made. We need a test that covers the scenario and passes with the fix in place (i.e. a test that would fail without your change)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed the PR to address this.

Copy link
Member

Choose a reason for hiding this comment

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

If you make the suggested change below, we can then update this test to verify that IsRunning is called multiple times, verifying that the polling loop is polling.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed this in updated commit

@mathewc
Copy link
Member

mathewc commented Oct 18, 2016

🚢

@soninaren soninaren merged commit 8a338dd into Azure:dev Oct 18, 2016
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.

5 participants