Skip to content

Add 'fail-fast' argument to flutter test #149587

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
Merged
auto-submit merged 3 commits into from
Jun 17, 2024
Merged

Add 'fail-fast' argument to flutter test #149587

auto-submit merged 3 commits into from
Jun 17, 2024

Conversation

@Lexycon
Copy link
Contributor

@Lexycon Lexycon commented Jun 3, 2024 •
edited
Loading

This adds the 'fail-fast' argument to flutter test, since dart test already supports this feature. Tests can now be stopped after first failure.

Fixes #124406

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].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jun 3, 2024
haevensk
haevensk reviewed Jun 3, 2024
View reviewed changes
Copy link

@haevensk haevensk left a comment

Choose a reason for hiding this comment

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

Still failing mac tests

@andrewkolos andrewkolos self-requested a review June 5, 2024 20:12
andrewkolos
andrewkolos requested changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@andrewkolos andrewkolos left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! It looks like this change doesn't update or add any tests. I think this is something we could put under test without much more maintenance burden. There are some existing tests that make sure other flags get piped along to package:test. You could draw inspiration from these to write one that makes sure --fail-fast is piped along properly. Alternatively, you could merge these tests together into one and then add --fail-fast to it. Let me know what you think.

@Lexycon
Copy link
Contributor Author

Lexycon commented Jun 7, 2024 •
edited
Loading

Thanks for the PR! It looks like this change doesn't update or add any tests. I think this is something we could put under test without much more maintenance burden. There are some existing tests that make sure other flags get piped along to package:test. You could draw inspiration from these to write one that makes sure --fail-fast is piped along properly. Alternatively, you could merge these tests together into one and then add --fail-fast to it. Let me know what you think.

Thank you for your response. I totally missed that there are already tests for flags which gets piped along and wasn't sure if this needs to be tested overall. I will take a look into the existing tests, maybe it really makes sense to merge them. Unfortunately, I am on vacation until mid next week, so this may take a bit. Thank you for your suggestions 😊

@Lexycon
Copy link
Contributor Author

Lexycon commented Jun 17, 2024 •
edited
Loading

Thanks for the PR! It looks like this change doesn't update or add any tests. I think this is something we could put under test without much more maintenance burden. There are some existing tests that make sure other flags get piped along to package:test. You could draw inspiration from these to write one that makes sure --fail-fast is piped along properly. Alternatively, you could merge these tests together into one and then add --fail-fast to it. Let me know what you think.

I saw this reporter group test got added a couple of days ago, so I basically did sth. similar and merged the tests (they basically do all the same) for --start-paused, --run-skipped, --test-randomize-ordering-seed=random and added the --fail-fast test.

There are still some tests which could be merged as well, like the --total-shards and --shard-index test. This one additionally checks that they are not piped by default, but ofc this could be implemented in the merged test group somehow as well.

But, probably we should keep it like it is now, a simple group for only simple passing checks. Let me know what you think.

@Lexycon Lexycon requested a review from andrewkolos June 17, 2024 11:37
@andrewkolos
Copy link
Contributor

andrewkolos commented Jun 17, 2024

I saw this reporter group test got added a couple of days ago, so I basically did sth. similar and merged the tests (they basically do all the same) for --start-paused, --run-skipped, --test-randomize-ordering-seed=random and added the --fail-fast test.

There are still some tests which could be merged as well, like the --total-shards and --shard-index test. This one additionally checks that they are not piped by default, but ofc this could be implemented in the merged test group somehow as well.

But, probably we should keep it like it is now, a simple group for only simple passing checks. Let me know what you think.

Ah, I hadn't noticed these tests. Yeah, this is one of the downsides of trying to group these into a single test. I think this is fine as it is; as long as the fail-fast flag is under test, I'm happy.

andrewkolos
andrewkolos approved these changes Jun 17, 2024
View reviewed changes
Copy link
Contributor

@andrewkolos andrewkolos left a comment

Choose a reason for hiding this comment

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

LGTM with nitpick

@Lexycon
Copy link
Contributor Author

Lexycon commented Jun 17, 2024

Thank you. Can you check your CLA? I think its missing for the PR.

@andrewkolos
Copy link
Contributor

andrewkolos commented Jun 17, 2024

Thank you. Can you check your CLA? I think its missing for the PR.

Yeah, I think this might be an issue with me using the GitHub suggestion feature. I'll investigate 👍

@andrewkolos
Copy link
Contributor

andrewkolos commented Jun 17, 2024

Added @jonahwilliams as a second reviewer since @christopherfujino is out-of-action for a couple of days

jonahwilliams
jonahwilliams approved these changes Jun 17, 2024
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewkolos andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 17, 2024
@auto-submit auto-submit bot merged commit 0287c22 into flutter:master Jun 17, 2024
@gnprice
Copy link
Member

gnprice commented Jun 17, 2024

Looking forward to using this feature — thanks @Lexycon for adding it! Glad the --reporter tests were helpful.

@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 18, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 18, 2024
@engine-flutter-autoroll
0287c22 Add 'fail-fast' argument to flutter test (flutter/flutter#149587
c0a7a95 
)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 18, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 18, 2024
@engine-flutter-autoroll
0287c22 Add 'fail-fast' argument to flutter test (flutter/flutter#149587
964a475 
)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 18, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 18, 2024
@engine-flutter-autoroll
0287c22 Add 'fail-fast' argument to flutter test (flutter/flutter#149587
a9f4364 
)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 18, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 18, 2024
@engine-flutter-autoroll
0287c22 Add 'fail-fast' argument to flutter test (flutter/flutter#149587
13401db 
)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 19, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 19, 2024
@engine-flutter-autoroll
0287c22 Add 'fail-fast' argument to flutter test (flutter/flutter#149587
4a609de 
)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 19, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 19, 2024
@engine-flutter-autoroll
0287c22 Add 'fail-fast' argument to flutter test (flutter/flutter#149587
012798d 
)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 20, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 20, 2024
@engine-flutter-autoroll
0287c22 Add 'fail-fast' argument to flutter test (flutter/flutter#149587
7629b26 
)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 20, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 20, 2024
@engine-flutter-autoroll
0287c22 Add 'fail-fast' argument to flutter test (flutter/flutter#149587
d621ea9 
)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 20, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 20, 2024
@engine-flutter-autoroll
0287c22 Add 'fail-fast' argument to flutter test (flutter/flutter#149587
8d2cd1c 
)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jun 20, 2024
@engine-flutter-autoroll
Manual roll Flutter from ccf3abe to 6c06abb (21 revisions) (#6954)
a958207 
Manual roll requested by [email protected]

flutter/flutter@ccf3abe...6c06abb

2024-06-18 [email protected] Add test for engine artifact framework permissions (flutter/flutter#148786)
2024-06-18 [email protected] Add test for icon_button.3.dart (flutter/flutter#149988)
2024-06-18 [email protected] Roll Flutter Engine from 78fdd06af541 to 74f42ca3544c (6 revisions) (flutter/flutter#150421)
2024-06-18 [email protected] Fix transparent `dividerColor` breaks `TabBar.tabAlignment` (flutter/flutter#150350)
2024-06-18 [email protected] Fix scrollable `TabBar` jittering (flutter/flutter#150041)
2024-06-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland 3: [CupertinoActionSheet] Match colors to native (#150386)" (flutter/flutter#150413)
2024-06-18 [email protected] Extend the Windows web_tool_tests_1_2 shard timeout to 45 minutes (flutter/flutter#150393)
2024-06-18 [email protected] Roll Flutter Engine from 1c4e5e230ecb to 78fdd06af541 (3 revisions) (flutter/flutter#150403)
2024-06-18 [email protected] Roll Flutter Engine from a4f266f7eb1a to 1c4e5e230ecb (8 revisions) (flutter/flutter#150399)
2024-06-18 [email protected] Rename doc file to use standard hyphens (flutter/flutter#150314)
2024-06-17 [email protected] Fix typo in `SliverLayoutDimensions.hashCode` where not all properties are used in the hash code. (flutter/flutter#150306)
2024-06-17 [email protected] Fix doc comment references to 'this' (flutter/flutter#150379)
2024-06-17 [email protected] Add 'fail-fast' argument to flutter test (flutter/flutter#149587)
2024-06-17 [email protected] Update matchesGoldenFile documentation reference to goldenFileComparator (flutter/flutter#150343)
2024-06-17 [email protected] Reland 3: [CupertinoActionSheet] Match colors to native (flutter/flutter#150386)
2024-06-17 [email protected] [a11y] Add semantics: button to bottom navigation bar items and dropdown menu items (flutter/flutter#149375)
2024-06-17 [email protected] Reland "sliverGridDelegate mainAxisExtent add assert (#148470)"  (flutter/flutter#149720)
2024-06-17 [email protected] `ScaffoldBackgroundColor` should default to `ColorScheme.surface` (flutter/flutter#149772)
2024-06-17 [email protected] Reland TreeSliver  (flutter/flutter#149839)
2024-06-17 [email protected] Reland: [CupertinoActionSheet] Add sliding tap gesture (flutter/flutter#150219)
2024-06-17 [email protected] Roll Flutter Engine from 5989f0215fed to a4f266f7eb1a (1 revision) (flutter/flutter#150377)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024
@engine-flutter-autoroll
0287c22 Add 'fail-fast' argument to flutter test (flutter/flutter#149587
f1b6c5a 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrewkolos andrewkolos andrewkolos approved these changes

+2 more reviewers

@haevensk haevensk haevensk left review comments

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App 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.

Add a "fast fail" argument to flutter test to stop after a failure

5 participants

@Lexycon @andrewkolos @gnprice @jonahwilliams @haevensk