Introduce a GN rule that (explicitly) generates a `dart test` wrapper #55475

matanlurey · 2024-09-27T01:55:10Z

This is a variant of the approach in #52241, based on feedback from @jakemac53 and @jonahwilliams (who originally sped up dart test significantly by using frontend_server under the scenes: dart-lang/test#1399), in short:

# tools/engine_tool/BUILD.gn

import("//flutter/build/dart/internal/gen_dartcli_call.gni")

gen_dartcli_call("tests") {
  args = [ "test" ]
  cwd = "//flutter/tools/engine_tool"
}

This stanza, when built (ninja -C ../out/host_debug flutter/tools/engine_tool:tests) generates the following file:

# ../out/host_debug/gen/flutter/tools/engine_tool/tests

set -e

# Needed because if it is set, cd may print the path it changed to.
unset CDPATH

# Store the current working directory.
prev_cwd=$(pwd)

# Set a trap to restore the working directory.
trap 'cd "$prev_cwd"' EXIT

CD_PATH="/Users/matanl/Developer/engine/src/flutter/tools/engine_tool"
if [ -n "$CD_PATH" ]; then
  cd "$CD_PATH"
fi

/Users/matanl/Developer/engine/src/flutter/prebuilts/macos-arm64/dart-sdk/bin/dart "test"

In turn, when executed (../out/host_debug/gen/flutter/tools/engine_tool/tests) it just runs dart test:

flutter % ../out/host_debug/gen/flutter/tools/engine_tool/tests
Building package executable... 
Built test:test.
00:00 +0: test/test_command_test.dart: test command executes test                                                                                                                                                                                                          
00:00 +3: test/run_command_test.dart: run command invokes flutter run
...
00:00 +117: All tests passed!

There is no actual compilation performed by the tool (that is handled implicitly by dart test), and as a result neither a depfile is needed, nor generating a pre-compiled artifact like a snapshot or AOT elf/assembly.

This work is incomplete, that is, we'd want to properly tag the executable so et can find it, and create a wrapper template (i.e. dart_test) that tightens things up a bit, but I wanted to show the work at this intermediate step to get feedback before moving forward.

/cc @jonahwilliams, @jtmcdole as well.

flutter-dashboard · 2024-09-27T01:55:14Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

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. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

jonahwilliams · 2024-09-27T02:00:50Z

build/dart/internal/gen_executable_call.py

+  script = string.Template(
+      '''#!/bin/sh
+
+set -e


dumb question because I get tripped up on this constantly. set -e makes sure that the exit code of this script is non zero if the dart test exit code is non zero right? Can you check that failed tests report non-zero exit codes through the shell script too?

Any command or pipeline that has an error will exit the script automatically. Dart tests that fail set errno 1.

jtmcdole · 2024-09-27T02:17:28Z

build/dart/internal/gen_executable_call.py

+  parser.add_argument('--output', required=True, help='Output file')
+  parser.add_argument('--command', required=True, help='Command to run')
+  parser.add_argument('--cwd', required=False, help='Working directory')
+  parser.add_argument('rest', nargs='*', help='Arguments to pass to the command')


I <3 rest vs args +1

zanderso · 2024-09-27T02:26:52Z

Does dart test do an implicit pub get?

matanlurey · 2024-09-27T02:35:28Z

Does dart test do an implicit pub get?

Yes, I believe it does. My thought that given how workspaces work, it shouldn't matter though?

(Side-note, if it does matter, we should make sure to change run_tests.py in the meantime).

jakemac53 · 2024-09-27T14:35:26Z

Yes, I believe it does. My thought that given how workspaces work, it shouldn't matter though?

Right, it shouldn't matter, but if we really wanted to skip it there might be a flag for that... cc @bkonyi .

bkonyi · 2024-09-27T14:55:18Z

Yes, I believe it does. My thought that given how workspaces work, it shouldn't matter though?

Right, it shouldn't matter, but if we really wanted to skip it there might be a flag for that... cc @bkonyi .

I don't think there's a flag to disable the automatic pub get since dart test triggers some precompilation steps before running tests. The argument handling for dart test is all from package:test and the precompilation logic is built into package:pub, so we'd need to add some logic there to support this (assuming we can).

jakemac53 · 2024-09-27T15:06:52Z

I don't think there's a flag to disable the automatic pub get since dart test triggers some precompilation steps before running tests.

As in the precompilation of the test package itself? If pub still has a run command, you should be able to just do pub run instead of pub get (it will also trigger the precompilation).

bkonyi · 2024-09-27T15:10:49Z

I don't think there's a flag to disable the automatic pub get since dart test triggers some precompilation steps before running tests.

As in the precompilation of the test package itself? If pub still has a run command, you should be able to just do pub run instead of pub get (it will also trigger the precompilation).

Yes, precompilation of the test package. dart test doesn't call pub run or pub get directly, it simply calls getExecutableForCommand('test:test') and passes the returned executable path back for the VM to run.

jakemac53 · 2024-09-27T15:17:40Z

Yes, precompilation of the test package. dart test doesn't call pub run or pub get directly, it simply calls getExecutableForCommand('test:test') and passes the returned executable path back for the VM to run.

It looks like that calls ensureUpToDate - which only does a pub get if necessary, which is probably a good thing in this case?

bkonyi · 2024-09-27T15:33:03Z

Yes, precompilation of the test package. dart test doesn't call pub run or pub get directly, it simply calls getExecutableForCommand('test:test') and passes the returned executable path back for the VM to run.

It looks like that calls ensureUpToDate - which only does a pub get if necessary, which is probably a good thing in this case?

Do we want to be running pub get at all on test infrastructure or are we relying on a custom package config to reference third_party/ dependencies and other local packages like we do in the SDK?

jakemac53 · 2024-09-27T15:56:11Z

Do we want to be running pub get at all on test infrastructure or are we relying on a custom package config to reference third_party/ dependencies and other local packages like we do in the SDK?

The engine (at least for the most part) uses pub workspaces, with overrides for the third party deps (pulled in via a DEPS file IIRC). https://github.com/flutter/engine/blob/main/pubspec.yaml. This means pub get is safe to do.

I would love the Dart SDK to also do it this way :).

jtmcdole

LGTM

cbracken · 2024-09-27T17:22:39Z

build/dart/internal/gen_executable_call.py

+unset CDPATH
+
+# Store the current working directory.
+prev_cwd=$$(pwd)


Could save yourself a variable with pushd "$CD_PATH" > /dev/null and popd > /dev/null in the exit signal handler. Also wouldn't need to worry about CDPATH above.

Done, that's much simpler!

Added a PWD "test" as well.

cbracken

matanlurey · 2024-09-27T17:57:12Z

I think @zanderso wanted a chance to weigh in here, and is OOO, so I'll check back with Monday before merging.

zanderso · 2024-09-28T21:32:30Z

Sorry, a couple more questions.

Is there any way to make the implied pub get run in --offline mode the same way that we do when setting up the tree in tools/pub_get_offline.py? I think this is not critical since the workspace should already be set up after a gclient sync, but I know folks forget to run gclient sync after doing a git pull all the time, and I'd like to avoid folks (and CI) getting their tree into a weird state.

Does dart test write to the source tree at all? If so, any files it writes will have to be cleaned up after the tests run, or we'll have to find somewhere else for it to write the files. For example, does it write any intermediate compilation artifacts anywhere? If it needs to do that, we'll have to find some way to avoid those being written in the source tree, or clean them up afterwords. Stray files in the source tree risk confusing the caching of the source tree that CI does.

matanlurey · 2024-09-29T01:47:36Z

Is there any way to make the implied pub get run in --offline mode the same way that we do when setting up the tree in tools/pub_get_offline.py? I think this is not critical since the workspace should already be set up after a gclient sync, but I know folks forget to run gclient sync after doing a git pull all the time, and I'd like to avoid folks (and CI) getting their tree into a weird state.

No, there is not. We could definitely request one if we end up thinking its important.

But, on the importance, I'm struggling a bit to understand what we're trying to avoid:

With workspace (and the enforcement of pub_get_offline.py) I'm not sure what it could do?
We already are using dart test, both locally and on CI.

Does dart test write to the source tree at all?

It does place files in .dart_tool/test:

But I'm guessing since it's in .gitignore we aren't considering that the source tree?

zanderso · 2024-09-30T14:56:18Z

With workspace (and the enforcement of pub_get_offline.py) I'm not sure what it could do?

If someone does a git pull without doing a gclient sync, then pub_get_offline.py won't have run. So my question is more, what is the failure mode if someone does git pull and then immediately dart test in a situation where the git pull updated the pubspec.yaml. Will it be easy to debug? Would an --offline flag to dart test make the failure more obvious? To be clear these questions are not intended to block this PR, but they are things we should be thinking about.

It does place files in .dart_tool/test:
But I'm guessing since it's in .gitignore we aren't considering that the source tree?

CI does filesystem level caching, so even files that are .gitignored in the source tree may persist between CI runs. Do we understand what the behavior of dart test is w.r.t. the files under .dart_tool/test when e.g. the version of package:test or the Dart SDK is updated?

Also a couple of questions for @bkonyi:

Does dart test send telemetry? Should we be explicitly disabling that on CI, somehow? Does dart test take a --disable-telemetry flag?
My understanding is that at some point in the near future, the Dart CLI will spawn a persistent compilation daemon. That means that these dart test invocation will spawn that process. Especially on CI, how will we be able to ensure that the daemons are killed between runs?

jtmcdole · 2024-09-30T16:00:22Z

CI does filesystem level caching, so even files that are .gitignored in the source tree may persist between CI runs

That's a surprising tidbit - is it documented somewhere? I would have imagined block level caching.

bkonyi · 2024-09-30T16:17:30Z

Also a couple of questions for @bkonyi:

Does dart test send telemetry? Should we be explicitly disabling that on CI, somehow? Does dart test take a --disable-telemetry flag?

The Dart CLI has a top-level --suppress-analytics flag that could be provided (e.g., dart --suppress-analytics test), but package:unified_analytics should be smart enough to detect if we're running on CI and disable telemetry automatically (detection logic found here).

My understanding is that at some point in the near future, the Dart CLI will spawn a persistent compilation daemon. That means that these dart test invocation will spawn that process. Especially on CI, how will we be able to ensure that the daemons are killed between runs?

@derekxu16 will know more about this since he's been the one working on it.

Would we want to keep the compilation server running and shared between tests on a shard? If not, there should be a way for the compilation server to have the same lifespan as the test process. If we do want to shared the compilation server, it should be as simple as adding a step to run dart compilation-server shutdown after all the tests have finished running.

jakemac53 · 2024-09-30T16:40:01Z

Do we understand what the behavior of dart test is w.r.t. the files under .dart_tool/test when e.g. the version of package:test or the Dart SDK is updated?

It is using the frontend server and passing a previous dill file to it to initialize from. The frontend server handles invalidating that as needed afaik (but if it doesn't do that, we have larger problems).

zanderso · 2024-09-30T16:51:48Z

CI does filesystem level caching, so even files that are .gitignored in the source tree may persist between CI runs

That's a surprising tidbit - is it documented somewhere? I would have imagined block level caching.

The recipes use https://flutter.googlesource.com/recipes/+/refs/heads/main/recipe_modules/cache/api.py, which reads/writes files to the CAS service.

johnmccutchan · 2024-09-30T18:09:57Z

Also a couple of questions for @bkonyi:

Does dart test send telemetry? Should we be explicitly disabling that on CI, somehow? Does dart test take a --disable-telemetry flag?

The Dart CLI has a top-level --suppress-analytics flag that could be provided (e.g., dart --suppress-analytics test), but package:unified_analytics should be smart enough to detect if we're running on CI and disable telemetry automatically (detection logic found here).

My understanding is that at some point in the near future, the Dart CLI will spawn a persistent compilation daemon. That means that these dart test invocation will spawn that process. Especially on CI, how will we be able to ensure that the daemons are killed between runs?

@derekxu16 will know more about this since he's been the one working on it.

Would we want to keep the compilation server running and shared between tests on a shard? If not, there should be a way for the compilation server to have the same lifespan as the test process. If we do want to shared the compilation server, it should be as simple as adding a step to run dart compilation-server shutdown after all the tests have finished running.

I think we need to be more careful than just shutting it down. We need these test runs to be hermetic. So, we can't have one test using a different tests' compilation server.

Is it possible to ensure that no other process connects to a compilation server?

derekxu16 · 2024-09-30T18:36:20Z

Is it possible to ensure that no other process connects to a compilation server?

Compilation servers will be associated with "info files", so it will be possible to specify which compilation server an invocation of dart test will connect to with the --resident-compiler-info-file CLI option. So passing a unique --resident-compiler-info-file argument to each invocation of dart test will result in those invocations being hermetic.

matanlurey · 2024-09-30T18:40:02Z

Let's wrap up the discussion on this PR about possible semantics of tools.

I opened 2 more issues to encourage discussion elsewhere:

Dirtying the source tree semantics #155949
dart test semantics on engine CI #155950

matanlurey · 2024-09-30T18:40:36Z

@johnmccutchan or @zanderso Do either of you have feedback specifically about this PR (i.e. should block merging)? If not, I plan to merge this shortly.

zanderso · 2024-09-30T19:16:04Z

@matanlurey LGTM w/ --suppress-analytics passed to dart test.

@derekxu16 @bkonyi Please keep the engine team and @johnmccutchan in the loop w.r.t. the persistent compilation daemon on CI.

…t` wrapper (flutter/engine#55475)

…155967) flutter/engine@e61bc85...bfb6ddd 2024-09-30 [email protected] Roll Skia from 534633fb4bd9 to d1d7deb68f9d (1 revision) (flutter/engine#55534) 2024-09-30 [email protected] Roll Dart SDK from 79863e31de87 to d9fb41a4b5ee (1 revision) (flutter/engine#55533) 2024-09-30 [email protected] Introduce a GN rule that (explicitly) generates a `dart test` wrapper (flutter/engine#55475) 2024-09-30 [email protected] Remove the need to use `runZoned` by replacing `print` statements (flutter/engine#55530) 2024-09-30 [email protected] Roll Skia from 6c89706638ee to 534633fb4bd9 (2 revisions) (flutter/engine#55531) 2024-09-30 [email protected] [Impeller] Create a libImpeller dylib and expose symbols. (flutter/engine#55526) 2024-09-30 [email protected] Added metal validation for `impeller_unittests (flutter/engine#55527) 2024-09-30 [email protected] Roll Skia from dfeeb199b226 to 6c89706638ee (2 revisions) (flutter/engine#55528) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-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

…lutter#155967) flutter/engine@e61bc85...bfb6ddd 2024-09-30 [email protected] Roll Skia from 534633fb4bd9 to d1d7deb68f9d (1 revision) (flutter/engine#55534) 2024-09-30 [email protected] Roll Dart SDK from 79863e31de87 to d9fb41a4b5ee (1 revision) (flutter/engine#55533) 2024-09-30 [email protected] Introduce a GN rule that (explicitly) generates a `dart test` wrapper (flutter/engine#55475) 2024-09-30 [email protected] Remove the need to use `runZoned` by replacing `print` statements (flutter/engine#55530) 2024-09-30 [email protected] Roll Skia from 6c89706638ee to 534633fb4bd9 (2 revisions) (flutter/engine#55531) 2024-09-30 [email protected] [Impeller] Create a libImpeller dylib and expose symbols. (flutter/engine#55526) 2024-09-30 [email protected] Added metal validation for `impeller_unittests (flutter/engine#55527) 2024-09-30 [email protected] Roll Skia from dfeeb199b226 to 6c89706638ee (2 revisions) (flutter/engine#55528) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-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

Follow-up to flutter#55475

Follow-up to #55475 Test: `flutter/tools/gn --no-prebuilt-dart-sdk --unoptimized --runtime-mode debug && ninja -C out/host_debug_unopt`

Follow-up to flutter/engine#55475 Test: `flutter/tools/gn --no-prebuilt-dart-sdk --unoptimized --runtime-mode debug && ninja -C out/host_debug_unopt`

matanlurey added 3 commits September 26, 2024 12:53

Start adding command_executable(...)

9c550c7

Generate a shell script.

f0d019d

++

9ae548a

matanlurey requested review from cbracken, jakemac53, johnmccutchan and zanderso September 27, 2024 01:55

jonahwilliams reviewed Sep 27, 2024

View reviewed changes

jtmcdole reviewed Sep 27, 2024

View reviewed changes

jtmcdole approved these changes Sep 27, 2024

View reviewed changes

jakemac53 approved these changes Sep 27, 2024

View reviewed changes

cbracken reviewed Sep 27, 2024

View reviewed changes

cbracken approved these changes Sep 27, 2024

View reviewed changes

Chris had good feedbacks.

5dc249c

matanlurey mentioned this pull request Sep 30, 2024

Document dirtying the source tree on CI (including .gitignore'd) has caching implications flutter/flutter#155949

Open

matanlurey mentioned this pull request Sep 30, 2024

Verify that dart test has acceptable semantics for engine CI flutter/flutter#155950

Closed

Tweak generation script.

ddce052

matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 30, 2024

auto-submit bot merged commit f5abfa0 into flutter:main Sep 30, 2024

engine-flutter-autoroll mentioned this pull request Sep 30, 2024

Roll Flutter Engine from e61bc853acb2 to bd36636b9a83 (7 revisions) flutter/flutter#155966

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 30, 2024

f5abfa06b Introduce a GN rule that (explicitly) generates a `dart tes…

178a636

…t` wrapper (flutter/engine#55475)

engine-flutter-autoroll mentioned this pull request Sep 30, 2024

Roll Flutter Engine from e61bc853acb2 to bfb6dddb2b30 (8 revisions) flutter/flutter#155967

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 30, 2024

f5abfa06b Introduce a GN rule that (explicitly) generates a `dart tes…

59a4a5c

…t` wrapper (flutter/engine#55475)

aam added a commit to aam/engine that referenced this pull request Oct 1, 2024

Fix gen_dartcli_call in --no-prebuilt-dart-sdk.

5c245a9

Follow-up to flutter#55475

aam mentioned this pull request Oct 1, 2024

Fix gen_dartcli_call in --no-prebuilt-dart-sdk. #55558

Merged

aam added a commit that referenced this pull request Oct 1, 2024

Fix gen_dartcli_call in --no-prebuilt-dart-sdk. (#55558)

d02a10f

Follow-up to #55475 Test: `flutter/tools/gn --no-prebuilt-dart-sdk --unoptimized --runtime-mode debug && ninja -C out/host_debug_unopt`

Introduce a GN rule that (explicitly) generates a dart test wrapper #55475

Introduce a GN rule that (explicitly) generates a dart test wrapper #55475

Uh oh!

Conversation

matanlurey commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Sep 27, 2024

Uh oh!

jonahwilliams Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

jtmcdole Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtmcdole Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

zanderso commented Sep 27, 2024

Uh oh!

matanlurey commented Sep 27, 2024

Uh oh!

jakemac53 commented Sep 27, 2024

Uh oh!

bkonyi commented Sep 27, 2024

Uh oh!

jakemac53 commented Sep 27, 2024

Uh oh!

bkonyi commented Sep 27, 2024

Uh oh!

jakemac53 commented Sep 27, 2024

Uh oh!

bkonyi commented Sep 27, 2024

Uh oh!

jakemac53 commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtmcdole left a comment

Choose a reason for hiding this comment

Uh oh!

cbracken Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matanlurey Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

matanlurey commented Sep 27, 2024

Uh oh!

zanderso commented Sep 28, 2024

Uh oh!

matanlurey commented Sep 29, 2024

Uh oh!

zanderso commented Sep 30, 2024

Uh oh!

jtmcdole commented Sep 30, 2024

Uh oh!

bkonyi commented Sep 30, 2024

Uh oh!

jakemac53 commented Sep 30, 2024

Uh oh!

zanderso commented Sep 30, 2024

Uh oh!

johnmccutchan commented Sep 30, 2024

Uh oh!

derekxu16 commented Sep 30, 2024

Uh oh!

matanlurey commented Sep 30, 2024

Uh oh!

matanlurey commented Sep 30, 2024

Uh oh!

zanderso commented Sep 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Introduce a GN rule that (explicitly) generates a `dart test` wrapper #55475

Introduce a GN rule that (explicitly) generates a `dart test` wrapper #55475

matanlurey commented Sep 27, 2024 •

edited

Loading

jtmcdole Sep 27, 2024 •

edited

Loading

jakemac53 commented Sep 27, 2024 •

edited

Loading

cbracken Sep 27, 2024 •

edited

Loading