Skip to content

[packages] Set parallelizable to NO to reduce test flakiness in packages project tests #6471

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
merged 2 commits into from
Apr 11, 2024
Merged

[packages] Set parallelizable to NO to reduce test flakiness in packages project tests #6471

merged 2 commits into from
Apr 11, 2024

Conversation

LouiseHsu
Copy link
Contributor

@LouiseHsu LouiseHsu commented Apr 5, 2024 •
edited
Loading

fixes flutter/flutter#146358

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 [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the [CLA].
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I [linked to at least one issue that this PR fixes] in the description above.
  • [] I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].
  • I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style].
  • 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.

@LouiseHsu LouiseHsu changed the title boop [packages] Set parallelizable to NO to reduce test flakiness in packages project tests Apr 5, 2024
@LouiseHsu LouiseHsu marked this pull request as ready for review April 5, 2024 19:04
@stuartmorgan-g
Copy link
Contributor

Do we always want this to be true in every project? If so, we should probably make a repo tool change that enforces it, so we don't invariably forget for new projects in the future.

@hellohuanlin
Copy link
Contributor

interesting. if setting to NO works, i suspect the flakiness may be due to our OCMock usage where we somehow created some sort of side effect by mistake.

@LouiseHsu
Copy link
Contributor Author

LouiseHsu commented Apr 5, 2024 •
edited
Loading

interesting. if setting to NO works, i suspect the flakiness may be due to our OCMock usage where we somehow created some sort of side effect by mistake.

this is probably true, but when it hangs its very difficult to debug, bc no stack trace or errors are generated. and if it just fails, the error/stack trace is suppressed too.

@LouiseHsu
Copy link
Contributor Author

Do we always want this to be true in every project? If so, we should probably make a repo tool change that enforces it, so we don't invariably forget for new projects in the future.

I feel like we probably should, unless you think there would be a significant performance trade off if the tests aren't running parallel. But most of the packages that I looked at dont have this attribute set at all, these are the only ones.

@LouiseHsu LouiseHsu requested a review from jmagman April 5, 2024 21:03
@jmagman
Copy link
Member

jmagman commented Apr 5, 2024

But most of the packages that I looked at dont have this attribute set at all

What's the Xcode default when you create a new project?

@LouiseHsu
Copy link
Contributor Author

But most of the packages that I looked at dont have this attribute set at all

What's the Xcode default when you create a new project?

parallelizable is set to YES by default when you create a new xcode project, but I think this is relatively new.

@stuartmorgan-g
Copy link
Contributor

interesting. if setting to NO works, i suspect the flakiness may be due to our OCMock usage where we somehow created some sort of side effect by mistake.

Global mocking in OCMock is a strong contender for the source of problems that only show up when running in parallel. That's one of several reasons I'm extremely against adding any new usage of it in the reviews I do.

@stuartmorgan-g
Copy link
Contributor

I'll file an issue for tooling enforcement of this; the PR can definitely land without the tooling part.

@stuartmorgan-g stuartmorgan-g mentioned this pull request Apr 9, 2024
Open
@jmagman jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 10, 2024
@auto-submit auto-submit bot merged commit be4964a into flutter:main Apr 11, 2024
1 check passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 12, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 12, 2024
@engine-flutter-autoroll
be4964a [packages] Set parallelizable to NO to reduce test flakiness …
c4b1245 
…in packages project tests (flutter/packages#6471)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 12, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 12, 2024
@engine-flutter-autoroll
be4964a [packages] Set parallelizable to NO to reduce test flakiness …
8757889 
…in packages project tests (flutter/packages#6471)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 12, 2024
@engine-flutter-autoroll
Roll Packages from e98839a to 78f684c (6 revisions) (#146691)
699749e 
flutter/packages@e98839a...78f684c

2024-04-12 [email protected] Roll Flutter from 557fbf5 to 53cba24 (11 revisions) (flutter/packages#6509)
2024-04-11 [email protected] [file_selector] Remove OCMock from iOS implementation (flutter/packages#6503)
2024-04-11 [email protected] Access current location using uri.path to support deep links (flutter/packages#6474)
2024-04-11 [email protected] Roll Flutter from 97cd47a to 557fbf5 (22 revisions) (flutter/packages#6502)
2024-04-11 [email protected] [packages] Set parallelizable to NO to reduce test flakiness in packages project tests (flutter/packages#6471)
2024-04-11 [email protected] Update multiple packages to depend on versions with iOS privacy manifest included (flutter/packages#6355)

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-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: 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
gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request Apr 22, 2024
@engine-flutter-autoroll @gilnobrega
Roll Packages from e98839a to 78f684c (6 revisions) (flutter#146691)
2e4e9d8 
flutter/packages@e98839a...78f684c

2024-04-12 [email protected] Roll Flutter from 557fbf5 to 53cba24 (11 revisions) (flutter/packages#6509)
2024-04-11 [email protected] [file_selector] Remove OCMock from iOS implementation (flutter/packages#6503)
2024-04-11 [email protected] Access current location using uri.path to support deep links (flutter/packages#6474)
2024-04-11 [email protected] Roll Flutter from 97cd47a to 557fbf5 (22 revisions) (flutter/packages#6502)
2024-04-11 [email protected] [packages] Set parallelizable to NO to reduce test flakiness in packages project tests (flutter/packages#6471)
2024-04-11 [email protected] Update multiple packages to depend on versions with iOS privacy manifest included (flutter/packages#6355)

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-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: 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
TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this pull request May 22, 2024
@LouiseHsu @TecHaxter
[packages] Set parallelizable to NO to reduce test flakiness in packa…
6596d61 
…ges project tests (flutter#6471)

fixes flutter/flutter#146358

- [] I updated `pubspec.yaml` with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
@LouiseHsu @arc-yong
[packages] Set parallelizable to NO to reduce test flakiness in packa…
df1f7c6 
…ges project tests (flutter#6471)

fixes flutter/flutter#146358

- [] I updated `pubspec.yaml` with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmagman jmagman jmagman approved these changes

@hellohuanlin hellohuanlin Awaiting requested review from hellohuanlin hellohuanlin is a code owner

@vashworth vashworth Awaiting requested review from vashworth

@loic-sharma loic-sharma Awaiting requested review from loic-sharma

@tarrinneal tarrinneal Awaiting requested review from tarrinneal tarrinneal is a code owner

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: google_sign_in p: pointer_interceptor p: video_player platform-ios platform-macos
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Set parallelizable to NO to reduce test flakiness in packages project tests
4 participants
@LouiseHsu @stuartmorgan-g @hellohuanlin @jmagman