[flutter_plugin_tool] Add custom-test command #5058
Conversation
Creates a repo-tool-based replacement for the "local_test.sh" script in flutter/packages. While ideally we don't want packages running bespoke tests (for maintenance reasons), in practice we do have a number of such tests and the script that runs them is strictly worse than the core of the repo tooling. This will allow us to eliminate "local_test.sh", and get all the benefits of the tooling (better changed package detection, run summaries, etc.) when running those custom tests. Part of flutter/flutter#85543
| @override | ||
| Future<PackageResult> runForPackage(RepositoryPackage package) async { | ||
| final File script = | ||
| package.directory.childDirectory('tool').childFile(_scriptName); |
There was a problem hiding this comment.
Pigeon has this in bin; I'll move it when I switch flutter/packages to this new command. Dart package layout docs say that bin is for package-client-facing commands, and scripts intended for package developers (the call out test scripts as an explicit example) belong in tool.
| } | ||
|
|
||
| // Run the legacy script if present. | ||
| if (legacyScript.existsSync()) { |
There was a problem hiding this comment.
This will make it so we run the dart script and the sh script. That will cause certain tests to get executed twice since the shell script delegates to the dart script. We should execute one or the other.
We could juggle things around in Pigeon so that the correct way is to execute the dart script and it will delegate to a shell script test that haven't been migrated (if it isn't windows). We could even make run_tests.sh just execute the dart script and there is another bash script the dart script delegates to for unmigrated tests.
There was a problem hiding this comment.
This will make it so we run the dart script and the sh script.
That's already how it works. It had to work that way in order to support pigeon's tests, which were partially implemented in one and partially implemented in the other.
That will cause certain tests to get executed twice since the shell script delegates to the dart script.
This code was supposed to prevent that. If Pigeon's test structure changed after that in a way that causes it to double-run tests, that's a Pigeon test bug.
We could juggle things around in Pigeon so that the correct way is to execute the dart script and it will delegate to a shell script test that haven't been migrated (if it isn't windows).
I don't have any opinion on how Pigeon manages the Dart vs shell split in the interim period until all tests are migrated, but it's orthogonal to this PR since the behavior isn't changing.
There was a problem hiding this comment.
That's already how it works.
Fair enough.
that's a Pigeon test bug.
I'm just trying to think what the fix is though given this logic and making sure we have a path forward. The easiest thing for pigeon would be to rename the script 'run_tests.sh' to 'shell_tests.sh' and make run_tests.dart execute shell_tests.sh. If pigeon isn't using run_tests.sh then probably no one is so we can get rid of this.
There was a problem hiding this comment.
I'm just trying to think what the fix is though given this logic and making sure we have a path forward.
I'm not clear on what the problem actually is. What is the flow where the code I linked to fails to avoid duplicate tests?
If pigeon isn't using
run_tests.shthen probably no one is so we can get rid of this.
Four packages currently use run_test.sh. See the linked issue for details.
Co-authored-by: gaaclarke <[email protected]>
| } | ||
|
|
||
| // Run the legacy script if present. | ||
| if (legacyScript.existsSync()) { |
There was a problem hiding this comment.
That's already how it works.
Fair enough.
that's a Pigeon test bug.
I'm just trying to think what the fix is though given this logic and making sure we have a path forward. The easiest thing for pigeon would be to rename the script 'run_tests.sh' to 'shell_tests.sh' and make run_tests.dart execute shell_tests.sh. If pigeon isn't using run_tests.sh then probably no one is so we can get rid of this.
|
This pull request is not suitable for automatic merging in its current state.
|
Creates a repo-tool-based replacement for the "local_test.sh" script in
flutter/packages. While ideally we don't want packages running bespoke
tests (for maintenance reasons), in practice we do have a number of such
tests and the script that runs them is strictly worse than the core of
the repo tooling. This will allow us to eliminate "local_test.sh", and
get all the benefits of the tooling (better changed package detection,
run summaries, etc.) when running those custom tests, as well as giving
test coverage for the command itself (unlike the shell script).
Part of flutter/flutter#85543
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).