Skip to content

Analyze against using Stopwatches in the framework #137805

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.

Sign up for GitHub

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

Jump to bottom
Closed
Piinks wants to merge 1 commit into from
Closed

Analyze against using Stopwatches in the framework #137805

Piinks wants to merge 1 commit into from

Conversation

@Piinks
Copy link
Contributor

@Piinks Piinks commented Nov 2, 2023

Adds analysis to prevent Stopwatches from being reintroduced to the framework.

Fixes #137804
This is a follow up to #137381
It will prevent issues like #135728, where adding a stopwatch introduced a high number of flakes in the framework when it fell out of sync with the clock. This particular case affected every call to fling something in testing, which meant over 300 tests were suddenly very flaky. 😱

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@Piinks Piinks added a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels. c: tech-debt Technical debt, code quality, testing, etc. labels Nov 2, 2023
@Piinks Piinks requested review from goderbauer and yjbanov November 2, 2023 22:58
goderbauer
goderbauer reviewed Nov 2, 2023
View reviewed changes
dev/bots/analyze.dart Outdated
Comment on lines +595 to +596
Copy link
Member

@goderbauer goderbauer Nov 2, 2023

Choose a reason for hiding this comment

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

Are we anticipating any legit reasons for opting out of this lint?

Copy link
Contributor Author

@Piinks Piinks Nov 2, 2023

Choose a reason for hiding this comment

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

Not sure yet, the analyzer just found a bunch for me to check out though!

Copy link
Contributor

@jonahwilliams jonahwilliams Nov 2, 2023

Choose a reason for hiding this comment

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

I think that the blinking cursor uses a periodic Timer (or it should) since the tick rate is only once every hundred or so ms and the timer avoids pumping empty frames.

dev/bots/analyze.dart Outdated
Copy link
Member

@goderbauer goderbauer Nov 2, 2023

Choose a reason for hiding this comment

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

@LongCatIsLooong was working on an analyzer-based way of detecting things like this instead of using somewhat error-prone string matching in https://github.com/flutter/flutter/pull/130101/files. This could be another great use case for that to exactly match on dart:core Stopwatch instantiations. Maybe we could extract the framework for these custom rules out of that PR and get the submitted without the more controversial debugAssert annotation so we could use it here as well?

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Nov 3, 2023

Choose a reason for hiding this comment

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

SG. I'll try converting this to using the analyzer package once that PR is ready.

Copy link
Contributor Author

@Piinks Piinks Nov 3, 2023

Choose a reason for hiding this comment

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

Awesome! Thank you @LongCatIsLooong!

@LongCatIsLooong LongCatIsLooong mentioned this pull request Nov 3, 2023
Closed
9 tasks
@github-actions github-actions bot added the f: gestures flutter/packages/flutter/gestures repository. label Nov 15, 2023
@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. engine flutter/engine related. See also e: labels. a: animation Animation APIs f: material design flutter/packages/flutter/material repository. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) a: internationalization Supporting other languages or locales. (aka i18n) f: scrolling Viewports, list views, slivers, etc. f: cupertino flutter/packages/flutter/cupertino repository d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: routes Navigator, Router, and related APIs. a: desktop Running on desktop f: integration_test The flutter/packages/integration_test plugin labels Nov 15, 2023
@Piinks Piinks removed the request for review from christopherfujino November 15, 2023 19:04
@Piinks
Copy link
Contributor Author

Piinks commented Nov 15, 2023

Strange rebase. Sorry for extra pings!

@Piinks
Copy link
Contributor Author

Piinks commented Nov 15, 2023

Bah. Will just reopen another PR, I don't know what happened and the reflog is mocking me. 🤣

@Piinks Piinks closed this Nov 15, 2023
@Piinks Piinks mentioned this pull request Nov 15, 2023
Merged
8 tasks
@piedcipher
Copy link
Member

piedcipher commented Nov 18, 2023

Strange rebase. Sorry for extra pings!

I faced a similar rebase issue in my very first pr for flutter/flutter.

#123775 (comment)

Workaround is to change the base branch to something else and then again back to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@goderbauer goderbauer goderbauer left review comments

@LongCatIsLooong LongCatIsLooong LongCatIsLooong left review comments

@yjbanov yjbanov Awaiting requested review from yjbanov

+1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) a: animation Animation APIs a: desktop Running on desktop a: internationalization Supporting other languages or locales. (aka i18n) a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems c: contributor-productivity Team-specific productivity, code health, technical debt. c: tech-debt Technical debt, code quality, testing, etc. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos engine flutter/engine related. See also e: labels. f: cupertino flutter/packages/flutter/cupertino repository f: gestures flutter/packages/flutter/gestures repository. f: integration_test The flutter/packages/integration_test plugin f: material design flutter/packages/flutter/material repository. f: routes Navigator, Router, and related APIs. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Prevent Stopwatches from being re-introduced to the framework

5 participants

@Piinks @piedcipher @goderbauer @jonahwilliams @LongCatIsLooong