CP-3485 Pub serve single instance for coverage #212
Conversation
There was a problem hiding this comment.
This looks really good, thank you for opening a PR @Anrock!
I just have a few comments, and then it looks like you'll need to run ddev format to fix up some formatting issues.
lib/src/tasks/coverage/api.dart
Outdated
| PubServeTask pubServeTask; | ||
| PubServeInfo serveInfo; | ||
|
|
||
| if (pubServe && pubServeTask == null) { |
There was a problem hiding this comment.
pubServeTask will always be null here since it is defined locally immediately before this
There was a problem hiding this comment.
Oops, forgot to remove this check. At first it meant to start pub serve on demand if current test is browser test and no pub serve was running. I'll fix this and other issues tommorow. Thanks for feedback.
lib/src/tasks/coverage/api.dart
Outdated
|
|
||
| // Build or modify the HTML file to properly load the test. | ||
| File _createHtmlForTest(File test, File customHtml) { |
There was a problem hiding this comment.
A slightly more accurate name here might be _createOrModifyHtmlForTest()
lib/src/tasks/coverage/api.dart
Outdated
| Future<List<int>> _runTestOnVm(String testPath) async { | ||
| const String _observatoryFailPattern = 'Could not start Observatory HTTP server'; | ||
| const String _testsFailedPattern = 'Some tests failed.'; | ||
| const String _testsPassedPattern = 'All tests passed!'; |
There was a problem hiding this comment.
Since these constants are used both here and in _runTestInBrowser() below, we should move them out so that they're only defined once.
|
@evanweible-wf fixed, please review. |
Codecov Report@@ Coverage Diff @@
## master #212 +/- ##
=======================================
Coverage 21.63% 21.63%
=======================================
Files 7 7
Lines 171 171
=======================================
Hits 37 37
Misses 134 134Continue to review full report at Codecov.
|
|
+1 looks great! If you're willing, we usually try to squash all the commits into 1 prior to merging. @Workiva/web-platform-pp |
|
@evanweible-wf squashed. |
|
+1 |
1 similar comment
|
+1 |
|
+10
|
|
@jayudey-wf ready for you |
rmconsole3-wf
changed the title
|
QA Resource Approval: +1
Merging into master. |
|
Thanks again for the contribution @Anrock! This is included in the latest release (1.7.2). |
Hello. This merge request changes coverage/api.dart so it uses single
pub serveinstance to run browser tests (if any) instead of starting and killingpub servefor each test. This reduces running time almost twice.