[flutter_tools] use frontend_server for web test compilation#70714
[flutter_tools] use frontend_server for web test compilation#70714jonahwilliams merged 18 commits intoflutter:masterfrom
Conversation
flutter-dashboard
bot
added
c: contributor-productivity
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
jonahwilliams
left a comment
jonahwilliams
left a comment
There was a problem hiding this comment.
TestOn chrome is not supported anymore, so we'll need to add the failing tests to the skip list
|
Gold has detected about 211 untriaged digest(s) on patchset 7. |
|
Gold has detected about 246 untriaged digest(s) on patchset 9. |
| globals.fs.path.join(outputDirectory.path, '${relativeTestSegments.join('_')}.test.dart')); | ||
| generatedFile | ||
| ..createSync(recursive: true) | ||
| ..writeAsStringSync(_generateEntrypoint(relativeTestSegments.join('/'), testFilePath)); |
There was a problem hiding this comment.
Would it be noticeably faster to fan out these writes into multiple concurrent writes and wait on them using Future.wait?
There was a problem hiding this comment.
This part of the process doesn't take much time - performance optimization is not a goal of this PR
| for (final File generatedFile in generatedFiles) { | ||
| buffer.writeln('import "${globals.fs.path.basename(generatedFile.path)}";'); | ||
| } | ||
| buffer.writeln('void main() {}'); |
There was a problem hiding this comment.
Can you please leave a comment explaining why the main.dart file has an empty main function?
| import '../project.dart'; | ||
| import '../web/compile.dart'; | ||
|
|
||
| /// A build_runner specific implementation of the [WebCompilationProxy]. |
There was a problem hiding this comment.
Does this comment need an update?
|
web_compilation_delegate.dart needs substantial refactoring/removing - but thought the line length doesn't show it this was a monster of a PR to get working, so I tried to keep most of the APIs the same. I have a lot of follow up work to do, but that should be much easier to get started on |
| .whereType<File>() | ||
| .map<String>((File file) => path.relative(file.path, from: flutterPackageDirectory.path)) | ||
| .where((String filePath) => !kWebTestFileKnownFailures.contains(filePath)) | ||
| .where((String filePath) => !kWebTestFileKnownFailures.contains(path.split(filePath).join('/'))) |
There was a problem hiding this comment.
allows running on windows
| @@ -1,14 +0,0 @@ | |||
| // Copyright 2014 The Flutter Authors. All rights reserved. | |||
There was a problem hiding this comment.
This is still unresolved: dart-lang/sdk#41764
| File metadataFile; | ||
| List<String> modules; | ||
| try { | ||
| final Directory parentDirectory = |
There was a problem hiding this comment.
Are you reformatting 80 character lines to longer? We're just fighting each other's autoformatting settings. We should get on the same page so we're not thrashing.
There was a problem hiding this comment.
I don't generally mind if you reformat things to be shorter, but that has to be a reciprocal agreement
There was a problem hiding this comment.
You know I wouldn't have any character limits if I were Boss. But I would rather just hit a button to format. We can talk about it outside this PR, but I don't think you even touched this file other than formatting it?
5 participants
Description
As a follow up, there is more refactoring required so that the bootstrap logic is shared between web run and test
Fixes #61120
Fixes #52685
Fixes #50594
Fixes #50594
Fixes #44583