remove unused divider#180990
Conversation
github-actions
bot
added
framework
|
@justinmc I split off the first part into this PR. As for the colors, I decided to add a test method, which uses permutations of |
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new set of widget test utilities, which will be valuable for writing tests that are independent of the Material library. The changes are clear and contribute to better test modularity. I have a couple of suggestions for the new utility file to enhance code clarity and maintainability. The removal of the unused Divider and the corresponding dependency cleanup is also a positive change.
| /// Get a random color for use in a widget test. | ||
| /// | ||
| /// The returned color will be fully opaque, | ||
| /// but the [Color.r], [Color.g], and [Color.b] channels | ||
| /// will each be randomized between `00` and `FF`. | ||
| Color getRandomTestColor(int index) { |
There was a problem hiding this comment.
The function name getRandomTestColor and its documentation are misleading. The function returns a deterministic color from a predefined list based on the index parameter, not a random one. This could lead to confusion for developers using this utility.
Consider renaming the function to something like getDeterministicTestColor and updating the documentation to reflect its deterministic nature.
/// Get a deterministic color for use in a widget test.
///
/// The returned color will be fully opaque, and is selected from a predefined
/// list of colors based on the given `index`.
Color getDeterministicTestColor(int index) {There was a problem hiding this comment.
I agree with this, but maybe just "getTestColor" is fine.
The word "random" is scary in the context of tests, because it makes me worry that subsequent runs of the same test might not always have the exact same result.
There was a problem hiding this comment.
Done. (see the other PR's also)
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
I'm sorry to nitpick the organization of your PR again, but rather than splitting this up into a "utils" changes PR and a "test" changes PR, could you split it by topic instead? Meaning for example, create a PR that both adds TestButton and uses it in the tests, and another PR that adds "getTestColor" and uses it in the tests.
As it is now, it's hard for me to understand exactly why each of these utils are needed and how they will be used without jumping back to the other PR.
Also, FYI I see there is another PR (#180456) that creates a TestWidgetsApp and is older than this PR. Let me try to get that one merged so that you can use it.
| /// Get a random color for use in a widget test. | ||
| /// | ||
| /// The returned color will be fully opaque, | ||
| /// but the [Color.r], [Color.g], and [Color.b] channels | ||
| /// will each be randomized between `00` and `FF`. | ||
| Color getRandomTestColor(int index) { |
There was a problem hiding this comment.
I agree with this, but maybe just "getTestColor" is fine.
The word "random" is scary in the context of tests, because it makes me worry that subsequent runs of the same test might not always have the exact same result.
There was a problem hiding this comment.
Nit: Maybe just name this file "utils.dart"? Since "widget" and "test" are in the path already.
There was a problem hiding this comment.
Done (See the other PR's also)
navaronbracke
force-pushed
the
widget_test_fixtures
branch
from
d0f5bda to
296ddb9
Compare
navaronbracke
changed the title
|
@justinmc I split off the relevant pieces into their own standalone PR's, the utils are now PR specific, instead of amending to different PR's |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
Thanks for putting up with all my nitpicks on the PR structure! This is a dead simple LGTM now 👍 . I'll take a look at the other PRs.
|
They aren't nitpicks if they make sense to do :p |
|
Can this get an autosubmit as well? |
bleroux
added
the
autosubmit
flutter-dashboard
bot
removed
the
autosubmit
This PR removes a single Divider from one test, since it was not adding anything noteworthy to the test. Part of flutter#177415 Part of flutter#180501 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
This PR removes a single Divider from one test, since it was not adding anything noteworthy to the test. Part of flutter#177415 Part of flutter#180501 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
5 participants
This PR removes a single Divider from one test, since it was not adding anything noteworthy to the test.
Part of #177415
Part of #180501
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.