Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Fix TaskRunnerTest.MaybeExecuteTaskOnlyExpired flake #29484

Merged
bdero merged 2 commits into from
Nov 2, 2021
Merged

Fix TaskRunnerTest.MaybeExecuteTaskOnlyExpired flake #29484

bdero merged 2 commits into from
Nov 2, 2021

Conversation

@bdero
Copy link
Member

@bdero bdero commented Nov 2, 2021 •
edited
Loading

Fixes flutter/flutter#92928.

This test was scheduling a task only 0.01ms into the future and verifying that it doesn't get executed (which may happen by the time runner.SimulateTimerAwake() samples the current time/depending on how precise it is).

@bdero bdero requested a review from zanderso November 2, 2021 19:38
@google-cla google-cla bot added the cla: yes label Nov 2, 2021
@zanderso
Copy link
Member

zanderso commented Nov 2, 2021

Depending on how the mock runner samples the time, the way this test is written could be inherently flaky. Maybe @moko256 can advise.

/cc @cbracken

@bdero
Copy link
Member Author

bdero commented Nov 2, 2021

Since a function ptr for sampling the time is passed to the task runner, I went ahead and removed the time sampling to make it fully deterministic.

cbracken
cbracken approved these changes Nov 2, 2021
View reviewed changes
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

lgtm -- thanks @bdero!

@bdero bdero merged commit 4e6f1de into flutter:master Nov 2, 2021
zanderso
zanderso reviewed Nov 2, 2021
View reviewed changes
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

lgtm

@moko256
Copy link
Contributor

moko256 commented Nov 3, 2021

Thanks @bdero!

The task_runner.cc is using std::chrono::steady_clock::now() to determine whether a task is expired. It is still not mocked up. To mock up it too will make the test more deterministic. I will send the PR later.

@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 3, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2021
@engine-flutter-autoroll
4e6f1de Fix TaskRunnerTest.MaybeExecuteTaskOnlyExpired flake (flutter…
ae2ca48 
…/engine#29484)
@moko256 moko256 mentioned this pull request Nov 3, 2021
Merged
8 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@zanderso zanderso zanderso left review comments

@cbracken cbracken cbracken approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

affects: tests cla: yes platform-windows

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Postsubmit windows shell flake: TaskRunnerTest.MaybeExecuteTaskOnlyExpired

4 participants

@bdero @zanderso @moko256 @cbracken