-
Notifications
You must be signed in to change notification settings - Fork 40
CP-1129 Add flag to automatically start pub serve when running tests/coverage
#92
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
lib/src/tasks/test/api.dart
Outdated
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.
I'd like to log some output here... At the very least, an indication that the pub server is starting, but ideally the pub serve output up until it starts.
Should pubServeTask be a field on TestTask, so that it can be accessed and logged accordingly in test/cli.dart?
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.
Yeah at this point, that's probably what I would do. You could break it up into two different parts - the logs up until the server is ready, and the remainder. That way you could print the first part immediately and the rest of it when the server is stopped.
|
@greglittlefield-wf I think your approach here is fine. As you've seen, the current dart_dev architecture makes this feel a bit hacky, and it's definitely something I want to improve in 2.0.0. But for now, this seems to me like it should work |
|
Cool, I'll proceed with this approach and work toward finalizing it. Thanks for the feedback! |
Current coverage is
|
pub serve when running testspub serve when running tests/coverage
|
This is ready for final review. Any clue as to why tests are timing out on Travis? They seem to run fine locally. |
lib/src/tasks/coverage/api.dart
Outdated
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.
Hmm, now that I think about it, if we're passing in additionalArgs: ['test'] to startPubServe, this stream will almost assuredly contain only one event.
This should probably be changed to:
PubServeInfo serveInfo = await pubServeTask.serveInfos.first;
if (!path.isWithin(serveInfo.directory, htmlFile.path)) {
throw '`pub serve` directory `${serveInfo.directory}` does not contain test file `${htmlFile.path}`.';
}|
Travis CI failure is an issue with Dart 1.13, I'm looking into it |
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.
The diff for this section is easier to look at if you ignore whitespace changes.
|
This should fix the CI failure once merged: #98 |
|
+1 |
|
@greglittlefield-wf This pull request has merge conflicts, please resolve. |
|
Merge conflicts have been resolved, and it looks like the build is passing now. Thanks, @evanweible-wf! |
|
@greglittlefield-wf This looks great! Could you add this config option to the readme? |
|
@evanweible-wf Yup! Added in 851ae16. |
|
+1 @trentgrover-wf @maxwellpeterson-wf @dustinlessard-wf any of you care to take a look as well? After that, last step would be a rebase and a squash into a single commit @greglittlefield-wf |
851ae16 to
cd82d2b
Compare
|
@evanweible-wf Rebased and squashed. |
|
+1 |
1 similar comment
|
+1 |
|
+1 cool |
|
+1 |
|
do you think we need to add an update to the readme about ensuring that you have this in your pubspec.yaml if you want to serve your tests? |
pub serve when running tests/coveragepub serve when running tests/coverage
|
@jayudey-wf That's a good idea, I'll add that. |
b19d342 to
223123f
Compare
0cbcb2e to
1855b61
Compare
|
K, added info to README about setting up @evanweible-wf Should I squash that commit in as well? |
|
@greglittlefield-wf sure, might as well. Readme changes look good |
1855b61 to
8b44e52
Compare
|
Squashed and pushed. |
|
+1 |
|
+1 @jayudey-wf ready for merge. |
|
QA Resource Approval: +10
Merging into master. |
CP-1129 Add flag to automatically start `pub serve` when running tests/coverage
Ultimate Problem
Tests and coverage currently cannot be run with files served from a Pub server.
See #79.
Solution
PubServeTaskpub serveinstances.pub-serveCLI option andpubServeconfig option to test/coverage tasks.dart_dev coverage/dart_dev testin projects that require a Pub server.Other changes
Tweak
Reporter.logGroupcode to be more flexible, and indent for multiline Stream events.Testing
test/coveragetasks pass.dart_dev test/dart_dev coverageusing a pub server.pub serveoutput is well-formatted.FYA: @evanweible-wf @trentgrover-wf
FYI: @jacehensley-wf @aaronlademann-wf