[tool] Only run unit tests in Chrome for inline web #4153

stuartmorgan-g · 2023-06-06T20:40:13Z

Currently we are running Dart unit tests in Chrome for any plugin with
web support, but it should only be necessary for plugins that have an
inline web implementation, not for app-facing packages that endorse a
web implementation.

Now that `google_maps_flutter` supports web, add it to the list of tests that don't currently run correctly on a Windows CI host.

Currently we are running Dart unit tests in Chrome for any plugin with web support, but it should only be necessary for plugins that have an inline web implementation, not for app-facing packages that endorse a web implementation.

flutter-dashboard · 2023-06-06T20:40:16Z

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.

stuartmorgan-g · 2023-06-06T20:42:29Z

This should work if I'm correctly understanding the reason for the "run in Chrome" code.

stuartmorgan-g · 2023-06-06T23:42:31Z

packages/video_player/video_player/test/video_player_test.dart

@@ -350,12 +350,12 @@ void main() {

      test('file with special characters', () async {
        final VideoPlayerController controller =
-            VideoPlayerController.file(File('A #1 Hit?.avi'));
+            VideoPlayerController.file(File('A #1 Hit.avi'));


It turns out ? is an illegal character in a Windows filename, so this test didn't work on Windows. The test still validates URL escaping without the ? so I just removed it rather than conditionalize by platform.

stuartmorgan-g · 2023-06-07T12:56:48Z

Everything is green; this should avoid that category of breakage in the future since endorsing web will no longer change the way we run unit tests in the app-facing package.

…packages#4153)

flutter/packages@a84b2c2...e13b8c4 2023-06-08 [email protected] [tool] Only run unit tests in Chrome for inline web (flutter/packages#4153) 2023-06-08 [email protected] [in_app_purchase] Make the _FeatureCard constructor const in the Android example app (flutter/packages#4162) 2023-06-08 [email protected] [shared_preferences] Fix initialization race (flutter/packages#4159) 2023-06-07 [email protected] [go_router] Refactors imperative APIs and browser history (flutter/packages#4134) 2023-06-07 [email protected] [various] Add `http` 1.0 compatibility (flutter/packages#4147) 2023-06-07 [email protected] [go_router_builder] Accept required parameters not in path (flutter/packages#4039) 2023-06-07 [email protected] Roll Flutter from 0b74153 to 8a5c22e (46 revisions) (flutter/packages#4160) 2023-06-07 [email protected] [pigeon] Require analyzer 5.13.0, prepare for NamedType refactoring. (flutter/packages#4127) 2023-06-07 [email protected] Roll Flutter (stable) from f92f441 to 682aa38 (1 revision) (flutter/packages#4157) 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://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

ditman · 2023-06-13T21:54:55Z

@stuartmorgan haven't we lost some test coverage with this change? AFAIK the unit tests shouldn't run on windows (because Chrome is missing there?), but they could still run on Linux (and maybe MacOS), right?

stuartmorgan-g · 2023-06-14T01:40:07Z

@stuartmorgan haven't we lost some test coverage with this change? AFAIK the unit tests shouldn't run on windows (because Chrome is missing there?), but they could still run on Linux (and maybe MacOS), right?

We shouldn't have lost any coverage unless there's something I'm not understanding about how we're using this flag.

The state before this PR was:

If a package is a plugin that supports web, run its Dart unit tests with --platform=chrome.
For any other situation, run its Dart unit tests normally (i.e., without --platform=chrome).

My understanding—correct me if I'm wrong here—is that we are only using --platform=chrome because of the way we are (were? I know you were converting things...) using Dart unit tests to run web-specific tests, which did things like import dart:html, such that they would only run within a web test host. But app-facing packages that just endorse a separate web plugin don't have any of those tests, so run fine without --platform=chrome.

The state after this PR is that app-facing packages that just endorse web implementation packages are now run without --platform=chrome instead of with. So

The same set of packages are tested on Linux (just not as many with --platform=chrome.
More packages are tested on Windows than were before.

ditman · 2023-06-15T21:42:28Z

The state before this PR was:

If a package is a plugin that supports web, run its Dart unit tests with --platform=chrome.

For any other situation, run its Dart unit tests normally (i.e., without --platform=chrome).

Ah, I misunderstood this. I thought that for packages that supported web, tests were run both normally (vm) and in Chrome (in linux).

If we're not doing the above, I think we should. Package cross_file relies on the behavior of both tests being run:

https://github.com/flutter/packages/tree/main/packages/cross_file/test

(Otherwise we're missing the web-specific tests, and that's what I meant by losing coverage).

stuartmorgan-g · 2023-06-15T23:58:01Z

If we're not doing the above, I think we should. Package cross_file relies on the behavior of both tests being run:

https://github.com/flutter/packages/tree/main/packages/cross_file/test

I filed flutter/flutter#128979, let me know if what I describe there sounds like the right logic.

stuartmorgan-g added 2 commits June 6, 2023 16:31

[ci] Exclude maps unit tests on Windows

f5e2b05

Now that `google_maps_flutter` supports web, add it to the list of tests that don't currently run correctly on a Windows CI host.

[tool] Only run unit tests in Chrome for inline web

210e1f2

Currently we are running Dart unit tests in Chrome for any plugin with web support, but it should only be necessary for plugins that have an inline web implementation, not for app-facing packages that endorse a web implementation.

stuartmorgan-g requested a review from ditman June 6, 2023 20:40

flutter-dashboard bot added the needs tests label Jun 6, 2023

Add unit test

94492b5

stuartmorgan-g removed the needs tests label Jun 6, 2023

Enable multidex in local_auth

ecf06e5

github-actions bot added p: local_auth platform-android labels Jun 6, 2023

Remove illegal character from test

4674122

stuartmorgan-g requested a review from tarrinneal as a code owner June 6, 2023 23:41

github-actions bot added the p: video_player label Jun 6, 2023

Merge branch 'main' into tooling-unit-test-web-inline-only

adbb685

github-actions bot removed p: local_auth platform-android labels Jun 6, 2023

stuartmorgan-g commented Jun 6, 2023

View reviewed changes

Re-remove google_maps_flutter from exceptions

f454adb

tarrinneal approved these changes Jun 7, 2023

View reviewed changes

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 8, 2023

auto-submit bot merged commit e13b8c4 into flutter:main Jun 8, 2023

engine-flutter-autoroll mentioned this pull request Jun 8, 2023

Roll Packages from a84b2c2ac344 to e13b8c43386a (9 revisions) flutter/flutter#128508

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 8, 2023

e13b8c4 [tool] Only run unit tests in Chrome for inline web (flutter/…

f1e7bf2

…packages#4153)

stuartmorgan-g deleted the tooling-unit-test-web-inline-only branch June 14, 2023 01:31

This was referenced Jul 7, 2023

[Tool] New tool to download android dependencies #4408

Merged

[camera] tree fix format 2023 05 12 #4450

Closed

[tooling] mark infra steps as infra steps in yaml files #4473

Merged

reidbaker mentioned this pull request Jul 26, 2023

[Tooling] Add google owned cache for dependencies as an option in ci #4567

Merged

11 tasks

reidbaker mentioned this pull request Aug 4, 2023

[All] Expand artifact hub to all plugins #4645

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[tool] Only run unit tests in Chrome for inline web #4153

[tool] Only run unit tests in Chrome for inline web #4153

Uh oh!

stuartmorgan-g commented Jun 6, 2023

Uh oh!

flutter-dashboard bot commented Jun 6, 2023

Uh oh!

stuartmorgan-g commented Jun 6, 2023

Uh oh!

stuartmorgan-g Jun 6, 2023

Uh oh!

stuartmorgan-g commented Jun 7, 2023

Uh oh!

ditman commented Jun 13, 2023 •

edited

Loading

Uh oh!

stuartmorgan-g commented Jun 14, 2023

Uh oh!

ditman commented Jun 15, 2023

Uh oh!

stuartmorgan-g commented Jun 15, 2023

Uh oh!

Uh oh!

[tool] Only run unit tests in Chrome for inline web #4153

[tool] Only run unit tests in Chrome for inline web #4153

Uh oh!

Conversation

stuartmorgan-g commented Jun 6, 2023

Uh oh!

flutter-dashboard bot commented Jun 6, 2023

Uh oh!

stuartmorgan-g commented Jun 6, 2023

Uh oh!

stuartmorgan-g Jun 6, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Jun 7, 2023

Uh oh!

ditman commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g commented Jun 14, 2023

Uh oh!

ditman commented Jun 15, 2023

Uh oh!

stuartmorgan-g commented Jun 15, 2023

Uh oh!

Uh oh!

ditman commented Jun 13, 2023 •

edited

Loading