-
Notifications
You must be signed in to change notification settings - Fork 348
Added enable flag for periodic task #901
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
base: develop
Are you sure you want to change the base?
Added enable flag for periodic task #901
Conversation
26b80b2 to
a4d60b2
Compare
core/src/utils/periodic_task.cpp
Outdated
| const auto settings = settings_.Read(); | ||
| taskEnabled = settings->enabled; | ||
| if(!taskEnabled) { | ||
| break; |
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.
Setting deadline + continue should be more straightforward and closer to existing code logic below
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.
what do you mean?
| task.Stop(); | ||
| } | ||
|
|
||
| UTEST(PeriodicTask, EnableDisable) { |
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.
Add a test that checks the task does not keep running periodically if ForceStepAsync is run while enabled=false
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.
This test will also check that ForceStepAsync ignores enabled
| task.Stop(); | ||
| } | ||
|
|
||
| UTEST(PeriodicTask, EnableDisable) { |
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.
There is a lot to decipher in this test's logic - split it into some steps with empty lines and comment on each step
| task.Stop(); | ||
| } | ||
|
|
||
| UTEST(PeriodicTask, EnableDisable) { |
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.
Add a test that checks that if enabled=false during Start (even if enabled=true in PeriodicTask constructor), then the first iteration won't happen immediately and won't happen after period
| task.Stop(); | ||
| } | ||
|
|
||
| UTEST(PeriodicTask, EnableDisable) { |
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.
Add a test that checks that SynchronizeDebug respects enabled
| task.Stop(); | ||
| } | ||
|
|
||
| UTEST(PeriodicTask, EnableDisable) { |
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.
Add a test that after enabled=false + enabled=on the task does not always start an iteration instantly, but awaits the configured interval since the last time it was run
Added enable flag for periodic task
Note: by creating a PR or an issue you automatically agree to the CLA. See CONTRIBUTING.md. Feel free to remove this note, the agreement holds.