Ash: Refactor notifications and harden test utilities [chromium/src : main]
Di Wu (Gerrit)
Di Wu added 1 comment![]( "Open in Gerrit")
notification_added_ = true;Di Wuwhy added? Shouldn't this be covered by OnNotificationAdded?
Hidehiko AbeGood question! I renamed the variable to make it less confusing. I also added a comment to explain why `OnNotificationUpdated` was added this time, in addition to the existing `OnNotificationAdded` callback. Please take a look.
Di WuI'm wondering if this is what we want.
Are we testing the behavior that a notification is newly added?
If it was, then we cannot confirm if some of the testing scenario actually triggered the notification intentionally, or something that is leaked from some prev set up?
Great point, per our discussion, please take a look at the latest design of the waiter that hopefully does a better job differentiating between different scenarios. Thanks!
Related details
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 3 comments![]( "Open in Gerrit")
waiter.WaitUntilRemoved();I do not recommend to reuse the waiter.
The removal event may happen before the timing we're looking at, which would be unexpected behavior.
Ditto for everywhere else.
// (`TaskEnvironment::TimeSource::MOCK_TIME`). In contrast, a manual `RunLoop`
// can sometimes cause unexpected side effects, such as prematurely flushing
// delayed tasks, leading to tests that pass for the wrong reasons (false
// positives).doesn't this mean the production code or test code would be wrong...?
Related details
- Di Wu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Wu (Gerrit)
Di Wu added 2 comments![]( "Open in Gerrit")
I do not recommend to reuse the waiter.
The removal event may happen before the timing we're looking at, which would be unexpected behavior.Ditto for everywhere else.
Good point. Updated, I found 5 places, all are in `chrome/browser/ash/policy/handlers/minimum_version_policy_handler_browsertest.cc`.
// (`TaskEnvironment::TimeSource::MOCK_TIME`). In contrast, a manual `RunLoop`
// can sometimes cause unexpected side effects, such as prematurely flushing
// delayed tasks, leading to tests that pass for the wrong reasons (false
// positives).doesn't this mean the production code or test code would be wrong...?
production code or test code would be wrong...
You have a sharp pair of eyes. I didn't find production code that was wrong. I did, however, find test code that was likely wrong, due to its false positive output.
See [here] for a timer that would fire a hats notification after a one-minute delay.
See [this original assertion] that would pass, without waiting out the whole one-minute delay, due to likely premature flushing of delayed tasks.
In a previous version of the waiter that I implemented, and that was never merged, I used RunLoop instead of RunUntil. I can confirm that the RunLoop version would always prematurely flush delayed tasks, at least in a mock time environment, just like its TestFuture counterpart. And the test would as a result pass with a false positive.
This is why RunUntil, which would not prematurely flush delayed tasks in a mock time environment, would need a companion `task_environment_.FastForwardBy(kDelayTriggerTimeout);` to go with it. Otherwise the test would time out and fail due to time would not automatically elapse in a mock time environment.
[here]:https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/webui/ash/cloud_upload/hats_office_trigger.cc;l=67
[this original assertion]:https://chromium-review.googlesource.com/c/chromium/src/+/6637692/30/chrome/browser/ui/webui/ash/cloud_upload/hats_office_trigger_unittest.cc#b308
Related details
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe voted and added 3 comments![]( "Open in Gerrit")
Votes added by Hidehiko Abe
| Code-Review | +1 |
3 comments
void CreateNotification();Hidehiko Abe`CreateNotification()` is absorbed in `ShowNotification()` to save an extra instance variable `notification_prompt_`.
Acknowledged
// (`TaskEnvironment::TimeSource::MOCK_TIME`). In contrast, a manual `RunLoop`
// can sometimes cause unexpected side effects, such as prematurely flushing
// delayed tasks, leading to tests that pass for the wrong reasons (false
// positives).Di Wudoesn't this mean the production code or test code would be wrong...?
production code or test code would be wrong...
You have a sharp pair of eyes. I didn't find production code that was wrong. I did, however, find test code that was likely wrong, due to its false positive output.
See [here] for a timer that would fire a hats notification after a one-minute delay.
See [this original assertion] that would pass, without waiting out the whole one-minute delay, due to likely premature flushing of delayed tasks.
In a previous version of the waiter that I implemented, and that was never merged, I used RunLoop instead of RunUntil. I can confirm that the RunLoop version would always prematurely flush delayed tasks, at least in a mock time environment, just like its TestFuture counterpart. And the test would as a result pass with a false positive.
This is why RunUntil, which would not prematurely flush delayed tasks in a mock time environment, would need a companion `task_environment_.FastForwardBy(kDelayTriggerTimeout);` to go with it. Otherwise the test would time out and fail due to time would not automatically elapse in a mock time environment.
[here]:https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/webui/ash/cloud_upload/hats_office_trigger.cc;l=67
[this original assertion]:https://chromium-review.googlesource.com/c/chromium/src/+/6637692/30/chrome/browser/ui/webui/ash/cloud_upload/hats_office_trigger_unittest.cc#b308
- Di Wu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Wu (Gerrit)
Di Wu added 1 comment![]( "Open in Gerrit")
Hi @osh...@chromium.org san, this is a follow-up in my code refactoring series of severing the ties between Cros code and the numerous tester/helper/utilities of DisplayService.
Especially, please help review the changes in `ui/message_center/test/message_center_waiter.h` and `ui/message_center/test/message_center_waiter.cc` as I further improved them to be even more specific and targeted when used in tests.
Let me know if you have any questions. Thanks!
Related details
- Mitsuru Oshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |