Fix that `flutter test` does not understand `concurrency` #125942

fzyzcjy · 2023-05-03T05:32:15Z

I will add tests if this PR looks roughly OK :)

The fix mainly mimics #115160 - just remove the default argument.

p.s. I ran into this bug when wanting to set concurrency in my dart_test.yaml for one set of my tests which I need to be executed without parallalization.

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.

flutter-dashboard · 2023-05-03T05:32:18Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

christopherfujino · 2023-05-04T20:59:24Z

Awesome, this approach looks good to me. If you add tests, we will review it next week.

fzyzcjy · 2023-05-04T23:40:45Z

Sure! I will add tests (in batch with my other PRs) as soon as possible when having time

fzyzcjy · 2023-05-05T07:16:02Z

Tests added (mimicking #115160), will come back and check CI soon

christopherfujino · 2023-05-05T16:41:26Z

@eliasyishak can you review this one?

eliasyishak · 2023-05-05T16:44:10Z

@eliasyishak can you review this one?

Yep I'll take a look!

eliasyishak · 2023-05-08T13:58:53Z

packages/flutter_tools/lib/src/commands/test.dart

@@ -146,7 +144,6 @@ class TestCommand extends FlutterCommand with DeviceBasedDevelopmentArtifacts {
      )
      ..addOption('concurrency',
        abbr: 'j',
-        defaultsTo: math.max<int>(1, globals.platform.numberOfProcessors - 2).toString(),


Although we are not setting the default value for concurrency in Flutter, we are eventually calling dart test which does have a default set for concurrency. As you can see here https://pub.dev/packages/test#test-concurrency, the default is going to be half of the host's CPU cores. ie. for me I have 10, so flutter would have defaulted to 8 while dart defaulted to 5.

So instead of removing the default help text, we should pass the default from dart.

This will prevent people from thinking that they are running on one thread when in reality it is likely running on a few.

Actually, I'm not 100% sure what I said above is correct, @christopherfujino, do you know if we are passing the args from the flutter command to dart test and using the dart defaults for test? Or do we have our own implementation that doesn't use the defaults?

Hmm, then how can we read data from dart_test.yaml? If we are passing a value like dart test --concurrency sth, dart test will ignore it, and thus the bug still occurs

Yep, to be clear I think the changes you made are correct in this PR, I just want to specify what the default is in the help message downstream

This testArgs list: https://github.com/flutter/flutter/pull/125942/files#diff-f3717e666fb03f50b2bb0b345f7d80d8ae1d07d7c07109ccd75ec8fdf5150ef2R112

Calls the main() function from package:test/executable.dart: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/test/test_wrapper.dart#L26

Which gets parsed here: https://github.com/dart-lang/test/blob/master/pkgs/test_core/lib/src/runner/configuration.dart#L94 (this is from the master branch)

I was referring to the defaultsTo value that gets printed along with the help text when you run flutter test --help. I'm suggesting that we keep the changes made in this PR but for the defaultsTo value, we should refer to the default value that dart uses, which is half of the number of processors on the developer's machine

I only skimmed this change, maybe I'm misunderstanding, but I thought the fix here was to remove defaultsTo and let package:test figure it out on its own, so that it will respect dart_test.yaml.

Yes, that is correct, and @fzyzcjy PR fixes that issue.

My only concern is that flutter users may not know that under the hood package:test also has a default value for concurrency that will be applied if the flutter developer does not supply a value for concurrency.

For example, if the user runs flutter test in their app without specifying concurrent workers, they may be thinking that it will only run on one worker, when in reality package:test is doing something else.

This may not be an issue but I thought giving that information in the help text would be helpful

But if we provide a defaultsTo value, then how would package:test ever look up dart_test.yaml? It will see that it's always being passed an explicit argument.

Ah this is my mistake, I forgot that defaultsTo will actually pass a value. Sorry for the confusion!

This should be okay then because we will end up with the same bug. I was really just trying to document that there will likely be more than one worker running for flutter test

eliasyishak

LGTM, thank you for contribution! 😄

christopherfujino

LGTM

auto-submit · 2023-05-08T18:57:09Z

auto label is removed for flutter/flutter, pr: 125942, Mergeability of pull request flutter/flutter/125942 could not be determined at time of merge..

Close #125940 I will add tests if this PR looks roughly OK :) The fix mainly mimics #115160 - just remove the default argument. p.s. I ran into this bug when wanting to set concurrency in my dart_test.yaml for one set of my tests which I need to be executed without parallalization.

…lutter/flutter#125942)

flutter/flutter@4ed1c92...8c5a1ea 2023-05-08 kevmoo@users.noreply.github.com tool/web: tiniest tweak to wasm help content (flutter/flutter#126284) 2023-05-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from d9184ce4ffa0 to 8d3a8162b3ab (7 revisions) (flutter/flutter#126288) 2023-05-08 5236035+fzyzcjy@users.noreply.github.com Fix that `flutter test` does not understand `concurrency` (flutter/flutter#125942) 2023-05-08 5236035+fzyzcjy@users.noreply.github.com Fix that `flutter test` does not understand `concurrency` (flutter/flutter#125942) 2023-05-08 github@alexv525.com Improve the format in `asset_bundle.dart` (flutter/flutter#126229) 2023-05-08 5236035+fzyzcjy@users.noreply.github.com Tiny cleanup for Navigator code (without introducing dependency) (flutter/flutter#125628) 2023-05-08 50433979+negotoyolo@users.noreply.github.com [Refactor] Fix style in example (flutter/flutter#126191) 2023-05-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from a8e9ac63fd95 to d9184ce4ffa0 (2 revisions) (flutter/flutter#126279) 2023-05-08 737941+loic-sharma@users.noreply.github.com [Tool] Output help on 'flutter pub' (flutter/flutter#126211) 2023-05-08 jacksongardner@google.com Always use `--concurrency=1` for web tests. (flutter/flutter#126179) 2023-05-08 42216813+eliasyishak@users.noreply.github.com Add `--verbose` flags for flakey tests (flutter/flutter#126162) 2023-05-08 engine-flutter-autoroll@skia.org Roll Packages from a0f8fd8 to 4800d65 (2 revisions) (flutter/flutter#126269) 2023-05-08 reidbaker@google.com Test AGP 8.0 using java 17 (flutter/flutter#125323) 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 camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…r#3949) flutter/flutter@4ed1c92...8c5a1ea 2023-05-08 kevmoo@users.noreply.github.com tool/web: tiniest tweak to wasm help content (flutter/flutter#126284) 2023-05-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from d9184ce4ffa0 to 8d3a8162b3ab (7 revisions) (flutter/flutter#126288) 2023-05-08 5236035+fzyzcjy@users.noreply.github.com Fix that `flutter test` does not understand `concurrency` (flutter/flutter#125942) 2023-05-08 5236035+fzyzcjy@users.noreply.github.com Fix that `flutter test` does not understand `concurrency` (flutter/flutter#125942) 2023-05-08 github@alexv525.com Improve the format in `asset_bundle.dart` (flutter/flutter#126229) 2023-05-08 5236035+fzyzcjy@users.noreply.github.com Tiny cleanup for Navigator code (without introducing dependency) (flutter/flutter#125628) 2023-05-08 50433979+negotoyolo@users.noreply.github.com [Refactor] Fix style in example (flutter/flutter#126191) 2023-05-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from a8e9ac63fd95 to d9184ce4ffa0 (2 revisions) (flutter/flutter#126279) 2023-05-08 737941+loic-sharma@users.noreply.github.com [Tool] Output help on 'flutter pub' (flutter/flutter#126211) 2023-05-08 jacksongardner@google.com Always use `--concurrency=1` for web tests. (flutter/flutter#126179) 2023-05-08 42216813+eliasyishak@users.noreply.github.com Add `--verbose` flags for flakey tests (flutter/flutter#126162) 2023-05-08 engine-flutter-autoroll@skia.org Roll Packages from a0f8fd8 to 4800d65 (2 revisions) (flutter/flutter#126269) 2023-05-08 reidbaker@google.com Test AGP 8.0 using java 17 (flutter/flutter#125323) 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 camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter/flutter#125942)

code

0880aa6

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label May 3, 2023

fix

a7aa3da

fzyzcjy and others added 3 commits May 5, 2023 15:12

fix linter

d0ca850

Merge branch 'master' into feat/fix-concurrency

b229970

add tests

b36d7b8

christopherfujino requested a review from eliasyishak May 5, 2023 16:41

eliasyishak reviewed May 8, 2023

View reviewed changes

eliasyishak approved these changes May 8, 2023

View reviewed changes

christopherfujino approved these changes May 8, 2023

View reviewed changes

christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label May 8, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 8, 2023

eliasyishak added the autosubmit Merge PR when tree becomes green via auto submit App label May 8, 2023

auto-submit bot merged commit 2a9530f into flutter:master May 8, 2023
125 checks passed

engine-flutter-autoroll mentioned this pull request May 9, 2023

Roll Flutter from 4ed1c9230310 to 8c5a1ea7281a (13 revisions) flutter/packages#3949

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 9, 2023

742a1b49e Fix that flutter test does not understand concurrency (f…

9e20b9d

…lutter/flutter#125942)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 9, 2023

2a9530f3f Fix that flutter test does not understand concurrency (f…

9765437

…lutter/flutter#125942)

HansMuller mentioned this pull request May 12, 2023

Flutter Framework Team: Notable Commits #121415

Open

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023

742a1b49e Fix that flutter test does not understand concurrency (f…

cf3ca95

…lutter/flutter#125942)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023

2a9530f3f Fix that flutter test does not understand concurrency (f…

9728c38

…lutter/flutter#125942)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

742a1b49e Fix that flutter test does not understand concurrency (f…

07538d5

…lutter/flutter#125942)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

2a9530f3f Fix that flutter test does not understand concurrency (f…

3bd70cb

…lutter/flutter#125942)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

742a1b49e Fix that flutter test does not understand concurrency (f…

b0a5fcc

…lutter/flutter#125942)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

2a9530f3f Fix that flutter test does not understand concurrency (f…

d14d21a

…lutter/flutter#125942)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

742a1b49e Fix that flutter test does not understand concurrency (f…

96e1ead

…lutter/flutter#125942)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

2a9530f3f Fix that flutter test does not understand concurrency (f…

cf7393a

…lutter/flutter#125942)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix that `flutter test` does not understand `concurrency` #125942

Fix that `flutter test` does not understand `concurrency` #125942

fzyzcjy commented May 3, 2023 •

edited

flutter-dashboard bot commented May 3, 2023

christopherfujino commented May 4, 2023

fzyzcjy commented May 4, 2023

fzyzcjy commented May 5, 2023 •

edited

christopherfujino commented May 5, 2023

eliasyishak commented May 5, 2023

eliasyishak May 8, 2023

eliasyishak May 8, 2023

fzyzcjy May 8, 2023

eliasyishak May 8, 2023

christopherfujino May 8, 2023

eliasyishak May 8, 2023

christopherfujino May 8, 2023

eliasyishak May 8, 2023

christopherfujino May 8, 2023

eliasyishak May 8, 2023

eliasyishak left a comment

christopherfujino left a comment

auto-submit bot commented May 8, 2023

Fix that flutter test does not understand concurrency #125942

Fix that flutter test does not understand concurrency #125942

Conversation

fzyzcjy commented May 3, 2023 • edited

Pre-launch Checklist

flutter-dashboard bot commented May 3, 2023

christopherfujino commented May 4, 2023

fzyzcjy commented May 4, 2023

fzyzcjy commented May 5, 2023 • edited

christopherfujino commented May 5, 2023

eliasyishak commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliasyishak left a comment

Choose a reason for hiding this comment

christopherfujino left a comment

Choose a reason for hiding this comment

auto-submit bot commented May 8, 2023

Fix that `flutter test` does not understand `concurrency` #125942

Fix that `flutter test` does not understand `concurrency` #125942

fzyzcjy commented May 3, 2023 •

edited

fzyzcjy commented May 5, 2023 •

edited