[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 5 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.github.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<Directory> _createTestDirectory(RepositoryPackage package) async { | ||
| const fileSystem = LocalFileSystem(); | ||
| final Directory testTempDirectory = await fileSystem.systemTempDirectory.createTemp(); |
There was a problem hiding this comment.
Using LocalFileSystem directly prevents the command from being unit-tested with a mock/memory file system. Use the file system from the package's directory (package.directory.fileSystem) instead.
| static Future<Directory> _createTestDirectory(RepositoryPackage package) async { | |
| const fileSystem = LocalFileSystem(); | |
| final Directory testTempDirectory = await fileSystem.systemTempDirectory.createTemp(); | |
| static Future<Directory> _createTestDirectory(RepositoryPackage package) async { | |
| final FileSystem fileSystem = package.directory.fileSystem; | |
| final Directory testTempDirectory = await fileSystem.systemTempDirectory.createTemp(); |
| 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:
|
1 participant
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