Migrate customer_testing to sharded tests.#138659
Migrate customer_testing to sharded tests.#138659auto-submit[bot] merged 20 commits intoflutter:masterfrom godofredoc:migrate_customer_testing
Conversation
dev/bots/test.dart
Outdated
There was a problem hiding this comment.
looks like git checkout HEAD is a no-op https://github.com/git/git/blob/ad7044857660af7ffaaf8fbbccc77b817d1b938f/builtin/checkout.c#L624 (HEAD is what we have currently checked out), thus let's only run the command if revision != null:
| final String revision = env['REVISION'] ?? 'HEAD'; | |
| await runCommand( | |
| 'git', | |
| <String>[ | |
| 'checkout', | |
| revision, | |
| ], | |
| workingDirectory: flutterRoot, | |
| ); | |
| final String? revision = env['REVISION']; | |
| if (revision != null) { | |
| await runCommand( | |
| 'git', | |
| const <String>[ | |
| 'checkout', | |
| revision, | |
| ], | |
| workingDirectory: flutterRoot, | |
| ); | |
| } |
There was a problem hiding this comment.
in what cases would env['REVISION'] not be set? just when run locally?
There was a problem hiding this comment.
looks like
git checkout HEADis a no-op (HEAD is what we have currently checked out), thus let's only run the command ifrevision != null:
Applied the suggestion.
TESTOWNERS
Outdated
There was a problem hiding this comment.
is this change effectively commenting out the customer_testing test owner logic? if so, why? if not, how does this work?
There was a problem hiding this comment.
Test owners validation is using "# " for shard validations.
.ci.yaml
Outdated
There was a problem hiding this comment.
so, we're expecting presubmits to not reflect this change, because compiling ci.yaml to lucicfg configs happens post-submit, right?
There was a problem hiding this comment.
That is correct, I've been running them manually with led https://luci-milo.appspot.com/raw/build/logs.chromium.org/flutter/led/godofredoc_google.com/c8307d6b5bb5f03b08393c1a5e1aaedc9ef2c2064b14521627832c38c09cc26a/+/build.proto
There was a problem hiding this comment.
This is the run for mac: https://chromium-swarm.appspot.com/task?id=6607d9ba068fc310
There was a problem hiding this comment.
And this one for win: https://chromium-swarm.appspot.com/task?id=6607dbd699281310
There was a problem hiding this comment.
looks like win is failing:
║ ProcessException: The system cannot find the file specified.
║
║ Command: ci.bat
║ #0 _ProcessImpl._start (dart:io-patch/process_patch.dart:402:33)
║ #1 Process.start (dart:io-patch/process_patch.dart:38:20)
║ #2 startCommand (file:///C:/b/s/w/ir/x/w/flutter/dev/bots/run_command.dart:97:47)
║ #3 runCommand (file:///C:/b/s/w/ir/x/w/flutter/dev/bots/run_command.dart:165:33)
║ #4 _runCustomerTesting (file:///C:/b/s/w/ir/x/w/flutter/dev/bots/test.dart:1583:9)
║ <asynchronous suspension>
║ #5 _runFromList (file:///C:/b/s/w/ir/x/w/flutter/dev/bots/test.dart:2175:5)
║ <asynchronous suspension>
║ #6 main (file:///C:/b/s/w/ir/x/w/flutter/dev/bots/test.dart:247:5)
║ <asynchronous suspension>
║
║ The test.dart script should be corrected to catch this error and call foundError().
║ Some tests are likely to have been skipped.
There was a problem hiding this comment.
Taking a look, I realized I didn't run on win before.
There was a problem hiding this comment.
The problem was that windows wants bat files to be executed using full path rather than a relative one.
This will allow Dart Team to run customer tests as part of monorepo. Bug: dart-lang/sdk#51042
dev/customer_testing/ci.bat
Outdated
There was a problem hiding this comment.
can you explain this diff? i have no batch foo.
There was a problem hiding this comment.
oh wait, nvm, i see the diff on find_commit.dart
There was a problem hiding this comment.
Is sending two more parameters to find_commit.
find_commit <first repo checkout in this case flutter/flutter> <branch of first repo (master)> <second repo checkout in this case flutter/tests> <branch of second repo(main)>
There was a problem hiding this comment.
ahh i see, confusingly in the old version, the first arg was optional. this LGTM
christopherfujino
left a comment
christopherfujino
left a comment
There was a problem hiding this comment.
LGTM, other than the failing windows LED build.
dev/customer_testing/ci.bat
Outdated
There was a problem hiding this comment.
ahh i see, confusingly in the old version, the first arg was optional. this LGTM
goderbauer
added
framework
godofredoc
added
the
autosubmit
godofredoc
added
the
autosubmit
This will allow Dart Team to run customer tests as part of monorepo and will be a step forward to remove ad_hoc tests. Bug: dart-lang/sdk#51042 Bug: flutter#115476
The migration of customer tests to sharded tests adds a step that checks out the current tip-of-tree of the framework repo, removing local changes. This does not work with monorepo testing, which modifies engine.version, and does not work with local testing of a branch. The sharded tests should already be running with the correct checkout of the framework repo. If the REVISION environment variable is set, the framework checkout will still be reset to check out that revision. These commands were migrated from the existing shell script to the sharded tester in flutter#138659 Bug: dart-lang/sdk#51042
|
The command that checks out HEAD of the framework repo if no REVISON env variable is set should not be needed when running as a sharded test. I have uploaded a PR to remove this as #141013 |
The migration of customer tests to sharded tests adds a step that checks out the current tip-of-tree of the framework repo, removing local changes. This does not work with monorepo testing, which modifies engine.version, and does not work with local testing of a branch. The sharded tests should already be running with the correct checkout of the framework repo. If the REVISION environment variable is set, the framework checkout will still be reset to check out that revision. These commands were migrated from the existing shell script to the sharded tester in #138659 Bug: dart-lang/sdk#51042
4 participants
This will allow Dart Team to run customer tests as part of monorepo and will be a step forward to remove ad_hoc tests.
Bug: dart-lang/sdk#51042
Bug: #115476
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.