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

      )
      ..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.

…utter/flutter#125942)

flutter/flutter@4ed1c92...8c5a1ea 2023-05-08 [email protected] tool/web: tiniest tweak to wasm help content (flutter/flutter#126284) 2023-05-08 [email protected] Roll Flutter Engine from d9184ce4ffa0 to 8d3a8162b3ab (7 revisions) (flutter/flutter#126288) 2023-05-08 [email protected] Fix that `flutter test` does not understand `concurrency` (flutter/flutter#125942) 2023-05-08 [email protected] Fix that `flutter test` does not understand `concurrency` (flutter/flutter#125942) 2023-05-08 [email protected] Improve the format in `asset_bundle.dart` (flutter/flutter#126229) 2023-05-08 [email protected] Tiny cleanup for Navigator code (without introducing dependency) (flutter/flutter#125628) 2023-05-08 [email protected] [Refactor] Fix style in example (flutter/flutter#126191) 2023-05-08 [email protected] Roll Flutter Engine from a8e9ac63fd95 to d9184ce4ffa0 (2 revisions) (flutter/flutter#126279) 2023-05-08 [email protected] [Tool] Output help on 'flutter pub' (flutter/flutter#126211) 2023-05-08 [email protected] Always use `--concurrency=1` for web tests. (flutter/flutter#126179) 2023-05-08 [email protected] Add `--verbose` flags for flakey tests (flutter/flutter#126162) 2023-05-08 [email protected] Roll Packages from a0f8fd8 to 4800d65 (2 revisions) (flutter/flutter#126269) 2023-05-08 [email protected] 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 [email protected],[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://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

…utter/flutter#125942)

…5942) Close flutter#125940 I will add tests if this PR looks roughly OK :) The fix mainly mimics flutter#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.

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

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

742a1b4 Fix that flutter test does not understand concurrency (fl…

9e20b9d

…utter/flutter#125942)

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

2a9530f Fix that flutter test does not understand concurrency (fl…

9765437

…utter/flutter#125942)

HansMuller mentioned this pull request May 12, 2023

Flutter Framework Team: Notable Commits #121415

Closed

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

742a1b4 Fix that flutter test does not understand concurrency (fl…

cf3ca95

…utter/flutter#125942)

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

2a9530f Fix that flutter test does not understand concurrency (fl…

9728c38

…utter/flutter#125942)

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

742a1b4 Fix that flutter test does not understand concurrency (fl…

07538d5

…utter/flutter#125942)

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

2a9530f Fix that flutter test does not understand concurrency (fl…

3bd70cb

…utter/flutter#125942)

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

742a1b4 Fix that flutter test does not understand concurrency (fl…

b0a5fcc

…utter/flutter#125942)

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

2a9530f Fix that flutter test does not understand concurrency (fl…

d14d21a

…utter/flutter#125942)

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

742a1b4 Fix that flutter test does not understand concurrency (fl…

96e1ead

…utter/flutter#125942)

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

2a9530f Fix that flutter test does not understand concurrency (fl…

cf7393a

…utter/flutter#125942)

Fix that flutter test does not understand concurrency #125942

Fix that flutter test does not understand concurrency #125942

Uh oh!

Conversation

fzyzcjy commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented May 3, 2023

Uh oh!

christopherfujino commented May 4, 2023

Uh oh!

fzyzcjy commented May 4, 2023

Uh oh!

fzyzcjy commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christopherfujino commented May 5, 2023

Uh oh!

eliasyishak commented May 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eliasyishak left a comment

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented May 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

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

fzyzcjy commented May 3, 2023 •

edited

Loading

fzyzcjy commented May 5, 2023 •

edited

Loading