[tools] Convert test utils to RepositoryPackage #5605
Conversation
Converts the test utils for creating fake packages and associated files
to use the newer `RepositoryPackage` instead of `Directory`, to improve
the typing of common objects.
As part of this, converts various test-local support utilities to take
`RepositoryPackage`s to keep the consistent type, and adds several more
package path utility methods for common subpaths to reduce duplication
of `directory.child{File,Directory}('some-hard-coded-value')`.
|
|
||
| /// Target platforms supported by Flutter. | ||
| // ignore: public_member_api_docs | ||
| enum FlutterPlatform { android, ios, linux, macos, web, windows } |
There was a problem hiding this comment.
I added this since I was adding a utility method to RepositoryPackage to get the platform directories of a package. Currently we use string constants everywhere for platforms which I've been wanting to move to enums, and this was a good opportunity to introduce it. (I didn't convert anything else to it yet.)
|
|
||
| /// Returns whether the specified entity is a directory containing a | ||
| /// `pubspec.yaml` file. | ||
| bool _isDartPackage(FileSystemEntity entity) { |
There was a problem hiding this comment.
This was a duplicate of the recently-added isPackage that I noticed while looking for uses of .childFile('pubspec.yaml').
| }); | ||
| }); | ||
|
|
||
| group('runXcodeBuild', () { |
There was a problem hiding this comment.
This was opportunistic copypasta fixing.
| const String _endColor = '\x1B[0m'; | ||
|
|
||
| // The filename within a package containing warnings to log during runForPackage. | ||
| enum _ResultFileType { |
There was a problem hiding this comment.
This and the utils below was opportunistic improvement of a bunch of places that would have needed to become package.directory.childFile..., with string constants, and I was trying to minimize that construction in the new RepositoryPackage world.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
LGTM, with a couple nits. I was reading through closely to look for clerical errors, but the scope of this PR is so large I'm not confident I didn't miss something. Ultimately the tests will be better at finding clerical errors, the change sounds good to me.
| case FlutterPlatform.windows: | ||
| directoryName = 'windows'; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Please add a default which throws an exception. I'm pretty sure incomplete switch statements are not an error in Dart, but our linter flags might catch it.
There was a problem hiding this comment.
We have the lint for that enabled in the repo, so if a new platform is added to the enum CI will fail unless this is updated.
| unorderedEquals(<String>[ | ||
| included.path, | ||
| included.childDirectory('example').path, | ||
| getExampleDir(included).path, |
There was a problem hiding this comment.
How come getExampleDir is a function when getting the platform directories is a method? That seems a bit odd.
There was a problem hiding this comment.
This is addressed in the getExampleDir comment:
/// This is deliberately not a method on [RepositoryPackage] since actual tool
/// code should essentially never need this, and instead be using
/// [RepositoryPackage.getExamples] to avoid assuming there's a single example
/// directory. However, needing to construct paths with the example directory
/// is very common in test code.
I considered making it a visable-for-testing method instead, but that seemed even more bizarre since it's not for testing RepositoryPackage. Making it a free function that's only defined in the test utilities means it'll never even show up as a possibility when people are writing non-test code.
There was a problem hiding this comment.
Not a huge deal. It just looked weird compared to the other methods. Could you have used extension methods in the test utilities?
There was a problem hiding this comment.
I also considered that, but it violates Flutter style.
stuartmorgan-g
added
the
waiting for tree to go green
3 participants
Converts the test utils for creating fake packages and associated files
to use the newer
RepositoryPackageinstead ofDirectory, to improvethe typing of common objects.
As part of this, converts various test-local support utilities to take
RepositoryPackages to keep the consistent type, and adds several morepackage path utility methods for common subpaths to reduce duplication
of
directory.child{File,Directory}('some-hard-coded-value').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.///).