-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Made snapshot rebuild logic take target into account #11730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
jakobr-google
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| final Iterable<File> files = inputPaths.map(fs.file); | ||
| /// input files and properties provided. | ||
| class Fingerprint { | ||
| Fingerprint.fromInputs({Set<String> filePaths, Map<String, String> properties}) : _fingerprint = <String, String>{} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: filePaths could just be an Iterable<String>, right? Then you wouldn't need to convert the lists to sets in the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| testUsingContext('throws if any input file does not exist', () async { | ||
| await fs.file('a.dart').create(); | ||
| expect(() => new Checksum.fromFiles(<String>['a.dart', 'b.dart'].toSet()), throwsA(anything)); | ||
| expect(() => new Fingerprint.fromInputs(filePaths: <String>['a.dart', 'b.dart'].toSet()), throwsA(anything)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throwsArgumentError instead of throwsA(anything)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| }); | ||
|
|
||
| testUsingContext('rebuilds if target changes', () async { | ||
| await runAsync(<String>['flutter', 'create', projectDir.path], allowReentrantFlutter: true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This'll only work if flutter is on the path (and maybe not on Windows). The other tests use createTestCommandRunner(), maybe that instead?
|
This appeared to have already been done by #11820, but that PR only takes change of main path into account, if the different main scripts are not part of the app for any other reason than being the main entry point. Specifically, if you have |
|
@cbracken will surely want to review this since this is his favourite part of the codebase. |
|
Superseded by #11924. |
Fixes #11400