[material_ui, cupertino_ui, go_router] Command to test dart fixes - #12390
[material_ui, cupertino_ui, go_router] Command to test dart fixes#12390justinmc wants to merge 12 commits into
Conversation
Adapted from go_router's run_tests script.
github-actions
Bot
added
p: go_router
triage-framework
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the standalone tool/run_tests.dart script with a new test-dart-fixes command in the repository tooling, implemented in TestDartFixes. This command automates running Dart fix tests for packages containing a test_fixes directory. Feedback on the implementation highlights opportunities to improve testability by using the package's file system and the configured processRunner instead of hardcoded LocalFileSystem and Process.start calls. Additionally, minor typos in string interpolation within error messages were identified.
| static Future<int> _runProcess( | ||
| String command, | ||
| List<String> arguments, { | ||
| String? workingDirectory, | ||
| }) async { | ||
| final Process process = await _streamOutput( | ||
| Process.start(command, arguments, workingDirectory: workingDirectory), | ||
| ); | ||
| return process.exitCode; | ||
| } |
There was a problem hiding this comment.
Calling Process.start directly from dart:io bypasses the processRunner passed to the constructor, which prevents mocking process execution in unit tests. Consider making these helper methods non-static and using processRunner to run the processes.
| try { | ||
| testDirectory = await _createTestDirectory(package); | ||
| } catch (error) { | ||
| return PackageResult.fail(['Failed to create temporary test directory: $error}']); |
There was a problem hiding this comment.
There is a typo in the string interpolation: $error} has an extra closing brace. It should be $error.
| return PackageResult.fail(['Failed to create temporary test directory: $error}']); | |
| return PackageResult.fail(['Failed to create temporary test directory: $error']); |
| } | ||
| result = PackageResult.success(); | ||
| } catch (error) { | ||
| result = PackageResult.fail(['Dart fix tests failed: $error}']); |
There was a problem hiding this comment.
There is a typo in the string interpolation: $error} has an extra closing brace. It should be $error.
| result = PackageResult.fail(['Dart fix tests failed: $error}']); | |
| result = PackageResult.fail(['Dart fix tests failed: $error']); |
|
Questions for @stuartmorgan-g in code review:
|
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Two main high-level things
- This command needs to be added to the CI so it's actually running. How long does it take to run? If it's fast we could just add it to https://github.com/flutter/packages/blob/main/.ci/targets/dart_unit_tests.yaml. If it's not, we'll need to think about where we can add it without causing issues.
- We unit test all of the repo tooling, so this needs corresponding tests in script/tool's test directory. See other commands for the common patterns of DI, faking process output/exit codes, etc., and let me know if you have any questions.
There are only 3 packages that support this, so it leaves a bunch of spam in the terminal saying "skipped x package". Is that ok or is there a way I should be avoiding that?
That's a feature :)
(For context, when I started working on this repo, I discovered that we had a significant number of tests that people thought we were running but weren't because of various mistakes that were silently ignored, and it was extremely hard to figure that out because the CI output was just a sea of results for the things that did run. As a result, the tool is now designed for auditability, so that it's very easy to look at a run log and see exactly what did and did not run, to make sure that matches expectations.)
I deleted go_router's script that did this previously, but is there anything else that I should clean up there?
I don't think so; removing run_tests.dart will cause the package to be skipped by the custom-tests command (in the same way that removing test_fixes would cause a package to be skipped by this new command).
| 'This command requires "flutter" to be in your path.'; | ||
|
|
||
| @override | ||
| PackageLoopingType get packageLoopingType => PackageLoopingType.includeAllSubpackages; |
There was a problem hiding this comment.
Could sub-packages ever have their own dart fixes? If not we only need to look at top-level packages.
There was a problem hiding this comment.
It's possible but doesn't happen in any of our packages. I'll set it to .topLevelOnly.
| return PackageResult.fail(['Failed to create temporary test directory: $error']); | ||
| } | ||
|
|
||
| late final PackageResult result; |
There was a problem hiding this comment.
Does this actually need the late keyword? I would expect it to work without it (with better safety).
| /// Run the dart fix tests for the package in the given temporary directory. | ||
| /// | ||
| /// Resolves with the status code of the command. | ||
| Future<int> _runDartFixTests(RepositoryPackage package, Directory testDirectory) async { |
There was a problem hiding this comment.
package is never used.
| final int pubGetStatusCode = await _runProcess('dart', <String>[ | ||
| 'pub', | ||
| 'get', | ||
| ], workingDirectory: testDirectory); |
There was a problem hiding this comment.
We have runPubGet to abstract this, in pub_utils.dart. You just need to construct a RepositoryPackage from the test directory.
| ], workingDirectory: testDirectory); | ||
| } | ||
|
|
||
| Future<int> _runProcess( |
There was a problem hiding this comment.
These methods should be removed in favor of the command taking a ProcessRunner, and using processRunner.runAndStream. See dart_test_command.dart for an example (or almost any other command).
About 40 seconds. Most of the time is from running |
2 participants
This PR creates a new repo-level CI command to run the dart fix tests in relevant packages. Before this PR, go_router had its own script to run its dart fix tests, and material_ui and cupertino_ui had dart fix tests that were not being run. After this PR, all of the above are run via the new command.
Part of flutter/flutter#182568