-
Notifications
You must be signed in to change notification settings - Fork 287
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
Fix Heartbeat Interval #1979
Fix Heartbeat Interval #1979
Conversation
@@ -49,7 +49,7 @@ public bool IsHeartbeatEnabled | |||
} | |||
|
|||
/// <summary> | |||
/// Gets or sets the delay interval between heartbeats. | |||
/// Gets or sets the delay interval between heartbeats. Setting this value will immediately reset the heartbeat timer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any way to unit test this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about this now. I'm reviewing our existing unit test coverage. Unit tests are passing locally, but I wanted this to run on our build server to confirm that all our integration tests still pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The System.Threading.Timer
is not easily testable. This timer doesn't expose the Interval property so can't inspect that it was changed.
We don't have proper test coverage of how heartbeats are created.
I think we would need to do some significant refactoring of this class so we can manually invoke a "timer event" and validate the telemetry that gets created.
I think that's out of scope for this PR.
I propose that we take these changes as-is and revisit this later.
Fixes issue of heartbeat interval not applying immediately when changed.
#1298
Changes
Checklist
For significant contributions please make sure you have completed the following items:
The PR will trigger build, unit tests, and functional tests automatically. Please follow these instructions to build and test locally.
Notes for authors:
Notes for reviewers:
/AzurePipelines run
will queue all builds/AzurePipelines run <pipeline-name>
will queue a specific build