Change ScheduledProcess's output streams over to be ScheduledStreams.

Change ScheduledProcess's output streams over to be ScheduledStreams.

This will help test "pub serve" with multiple entrypoints.

R=rnystrom@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=32770

[Bob Nystrom, 2014-02-14 17:55:02]

This is a really nice improvement.

https://codereview.chromium.org/164773003/diff/30001/pkg/scheduled_test/lib/s...
File pkg/scheduled_test/lib/scheduled_process.dart (right):

https://codereview.chromium.org/164773003/diff/30001/pkg/scheduled_test/lib/s...
pkg/scheduled_test/lib/scheduled_process.dart:265: /// [remainingStdout], and
[remainingStderr].
Remove references to the deleted methods (elsewhere too if there are any).

Is it worth even keeping this in the public API anymore? Since this is a
breaking change, now might be a good time to remove it.

https://codereview.chromium.org/164773003/diff/30001/pkg/scheduled_test/lib/s...
File pkg/scheduled_test/lib/src/stream_matcher.dart (right):

https://codereview.chromium.org/164773003/diff/30001/pkg/scheduled_test/lib/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:305: if (description != null) {
It took me a long time to figure out what's going on here. How about calling
"description" "failure" to hint that its presence indicates failure? I know
StreamMatcher's docs for tryMatch() clarify this, but it's a drag to have to go
there and find them.

Another option would be have tryMatch() throw on failure instead of return?

https://codereview.chromium.org/164773003/diff/30001/pkg/scheduled_test/pubsp...
File pkg/scheduled_test/pubspec.yaml (right):

https://codereview.chromium.org/164773003/diff/30001/pkg/scheduled_test/pubsp...
pkg/scheduled_test/pubspec.yaml:2: version: 0.10.0-dev
Is it worth publishing 0.9.3 before this lands?

https://codereview.chromium.org/164773003/diff/30001/pkg/scheduled_test/test/...
File pkg/scheduled_test/test/scheduled_process_test.dart (right):

https://codereview.chromium.org/164773003/diff/30001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/scheduled_process_test.dart:133:
process.stdout.expect('world');
So much better!

https://codereview.chromium.org/164773003/diff/30001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/scheduled_process_test.dart:168: expectTestsPass("stdout
exposes the stdandard output from the process", () {
"stdandard" -> "standard"

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/lish/preview_package_validation_has_a_warning_test.dart
(right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/lish/preview_package_validation_has_a_warning_test.dart:31:
'\n'
It might be cleaner just to use a multi-line string here.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/lish/upload_form_provides_invalid_json_test.dart
(right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/lish/upload_form_provides_invalid_json_test.dart:29:
'{not json'));
In most other places, you seem to do a separate pub.stderr.expect call for each
line. Why not here?

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/oauth2/with_a_server_rejected_refresh_token_authenticates_again_test.dart
(right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/oauth2/with_a_server_rejected_refresh_token_authenticates_again_test.dart:44:
pub.stdout.expect(matches(r'Uploading\.\.\.+'));
Use the startsWith matcher.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/pub_uploader_test.dart (right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/pub_uploader_test.dart:169: '{not json'));
Be consistent about using emitsString() or multiple calls to .stderr.expect.
(I'd just use multiple calls.)

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve/utils.dart:143:
_pubServer.stdout.expect(startsWith("Downloading"));
I hacked in the numDownloads stuff because at the time there was no easy way to
skip an unknown number of lines. You can probably delete all of this and just
do:

_pubServer.stdout.expect(consumeThrough("Got dependencies!"));

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve/utils.dart:223:
_pubServer.stdout.expect(consumeThrough(contains("successfully")));
\o/

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/serve/warns_on_assets_paths_test.dart (right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve/warns_on_assets_paths_test.dart:20: r'^Warning:
Pub reserves paths containing "assets" for using assets from '
Given that the second line intentionally isn't raw, why make this one?

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/test_pub.dart:446: "Looks great! Are you ready to
upload your package (y/n)?"));
Multi-line string?

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/test_pub.dart:511: /// [remainingStderr] work as
though pub weren't running in verbose mode.
Update docs.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/test_pub.dart:577: if (entry.first != log.Level.ERROR
&& entry.first != log.Level.WARNING) {
How'd this long line sneak in?

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/test_pub.dart:871: StreamMatcher emitsString(String
output) => inOrder(output.split("\n"));
How about "emitsLines"?

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/transformer/dart2js/does_not_support_invalid_command_line_options_type_test.dart
(right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/transformer/dart2js/does_not_support_invalid_command_line_options_type_test.dart:38:
'Build error:\n'
Why use emitsString here (and in the later changes)?

[nweiz, 2014-02-18 22:01:10]

https://codereview.chromium.org/164773003/diff/30001/pkg/scheduled_test/lib/s...
File pkg/scheduled_test/lib/scheduled_process.dart (right):

https://codereview.chromium.org/164773003/diff/30001/pkg/scheduled_test/lib/s...
pkg/scheduled_test/lib/scheduled_process.dart:265: /// [remainingStdout], and
[remainingStderr].
On 2014/02/14 17:55:02, Bob Nystrom wrote:
> Remove references to the deleted methods (elsewhere too if there are any).

Done.

> Is it worth even keeping this in the public API anymore? Since this is a
> breaking change, now might be a good time to remove it.

This is necessary for the pub tests. They use it to get a "side channel" log of
the entire process output, and they override them to interpret the verbose mode
output as though it were normal stdout/stderr.

https://codereview.chromium.org/164773003/diff/30001/pkg/scheduled_test/lib/s...
File pkg/scheduled_test/lib/src/stream_matcher.dart (right):

https://codereview.chromium.org/164773003/diff/30001/pkg/scheduled_test/lib/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:305: if (description != null) {
On 2014/02/14 17:55:02, Bob Nystrom wrote:
> It took me a long time to figure out what's going on here. How about calling
> "description" "failure" to hint that its presence indicates failure? I know
> StreamMatcher's docs for tryMatch() clarify this, but it's a drag to have to
go
> there and find them.

I'll do this in a follow-up CL so I can change it throughout the file.

> Another option would be have tryMatch() throw on failure instead of return?

That would make it more difficult to compose the error messages, and it would be
a worse fit with the existing unittest API.

https://codereview.chromium.org/164773003/diff/30001/pkg/scheduled_test/pubsp...
File pkg/scheduled_test/pubspec.yaml (right):

https://codereview.chromium.org/164773003/diff/30001/pkg/scheduled_test/pubsp...
pkg/scheduled_test/pubspec.yaml:2: version: 0.10.0-dev
On 2014/02/14 17:55:02, Bob Nystrom wrote:
> Is it worth publishing 0.9.3 before this lands?

Sure, I'll do that.

https://codereview.chromium.org/164773003/diff/30001/pkg/scheduled_test/test/...
File pkg/scheduled_test/test/scheduled_process_test.dart (right):

https://codereview.chromium.org/164773003/diff/30001/pkg/scheduled_test/test/...
pkg/scheduled_test/test/scheduled_process_test.dart:168: expectTestsPass("stdout
exposes the stdandard output from the process", () {
On 2014/02/14 17:55:02, Bob Nystrom wrote:
> "stdandard" -> "standard"

Done.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/lish/preview_package_validation_has_a_warning_test.dart
(right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/lish/preview_package_validation_has_a_warning_test.dart:31:
'\n'
On 2014/02/14 17:55:02, Bob Nystrom wrote:
> It might be cleaner just to use a multi-line string here.

That's contrary to how we usually do long multiline strings. Why do you think a
multiline string would be a good fit here and not elsewhere?

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/lish/upload_form_provides_invalid_json_test.dart
(right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/lish/upload_form_provides_invalid_json_test.dart:29:
'{not json'));
On 2014/02/14 17:55:02, Bob Nystrom wrote:
> In most other places, you seem to do a separate pub.stderr.expect call for
each
> line. Why not here?

The places with two expect calls have `JSON.encode(body)` as the second
expression rather than a literal string, so I thought the separate calls were a
little cleaner than interpolation. I could switch them to interpolation if you
prefer that.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/oauth2/with_a_server_rejected_refresh_token_authenticates_again_test.dart
(right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/oauth2/with_a_server_rejected_refresh_token_authenticates_again_test.dart:44:
pub.stdout.expect(matches(r'Uploading\.\.\.+'));
On 2014/02/14 17:55:02, Bob Nystrom wrote:
> Use the startsWith matcher.

Done.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/pub_uploader_test.dart (right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/pub_uploader_test.dart:169: '{not json'));
On 2014/02/14 17:55:02, Bob Nystrom wrote:
> Be consistent about using emitsString() or multiple calls to .stderr.expect.
> (I'd just use multiple calls.)

I chose [emitsString] for consistency with longer multiline expectations, where
multiple calls to [expect] get very cumbersome. Previously we had been using
multiple calls by necessity, since there was no facility for matching multiple
lines in a single call, but now we do so I thought it would be cleaner to use
it.

I can switch to multiple calls if you don't mind the inconsistency between
two-line expectations and many-line expectations.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve/utils.dart:143:
_pubServer.stdout.expect(startsWith("Downloading"));
On 2014/02/14 17:55:02, Bob Nystrom wrote:
> I hacked in the numDownloads stuff because at the time there was no easy way
to
> skip an unknown number of lines. You can probably delete all of this and just
> do:
> 
> _pubServer.stdout.expect(consumeThrough("Got dependencies!"));

Done.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/serve/warns_on_assets_paths_test.dart (right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve/warns_on_assets_paths_test.dart:20: r'^Warning:
Pub reserves paths containing "assets" for using assets from '
On 2014/02/14 17:55:02, Bob Nystrom wrote:
> Given that the second line intentionally isn't raw, why make this one?

I think RegExp strings should be raw wherever possible so people don't have to
think about to escape a character or not every time they write a regexp. I'd
almost rather the next line be

    r'packages\. Please rename the path "' + assetsPath + r'\.$'

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/test_pub.dart:446: "Looks great! Are you ready to
upload your package (y/n)?"));
On 2014/02/14 17:55:02, Bob Nystrom wrote:
> Multi-line string?

Same response as before -- why here and not elsewhere?

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/test_pub.dart:511: /// [remainingStderr] work as
though pub weren't running in verbose mode.
On 2014/02/14 17:55:02, Bob Nystrom wrote:
> Update docs.

Done.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/test_pub.dart:577: if (entry.first != log.Level.ERROR
&& entry.first != log.Level.WARNING) {
On 2014/02/14 17:55:02, Bob Nystrom wrote:
> How'd this long line sneak in?

Fixed.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/test_pub.dart:871: StreamMatcher emitsString(String
output) => inOrder(output.split("\n"));
On 2014/02/14 17:55:02, Bob Nystrom wrote:
> How about "emitsLines"?

I considered that name, but it felt weird that it took a string rather than
lines. I can change it if emitsLines feels more sensible to you, though.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/transformer/dart2js/does_not_support_invalid_command_line_options_type_test.dart
(right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/transformer/dart2js/does_not_support_invalid_command_line_options_type_test.dart:38:
'Build error:\n'
On 2014/02/14 17:55:02, Bob Nystrom wrote:
> Why use emitsString here (and in the later changes)?

Same reason as before: for consistency with longer multiline expectations.

[Bob Nystrom, 2014-02-18 22:12:59]

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/lish/preview_package_validation_has_a_warning_test.dart
(right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/lish/preview_package_validation_has_a_warning_test.dart:31:
'\n'
On 2014/02/18 22:01:10, nweiz wrote:
> On 2014/02/14 17:55:02, Bob Nystrom wrote:
> > It might be cleaner just to use a multi-line string here.
> 
> That's contrary to how we usually do long multiline strings.

Is that true? We have a decent number of tests with multiline strings.

> Why do you think a multiline string would be a good fit here and not
elsewhere?

Here you have a string that's four lines long and has a bunch of explicit
newlines. Since the string literal itself is already spread across multiple
lines, you may as well use that to your advantage and use the implicit newlines
that a multiline string gives you.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/lish/upload_form_provides_invalid_json_test.dart
(right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/lish/upload_form_provides_invalid_json_test.dart:29:
'{not json'));
On 2014/02/18 22:01:10, nweiz wrote:
> On 2014/02/14 17:55:02, Bob Nystrom wrote:
> > In most other places, you seem to do a separate pub.stderr.expect call for
> each
> > line. Why not here?
> 
> The places with two expect calls have `JSON.encode(body)` as the second
> expression rather than a literal string, so I thought the separate calls were
a
> little cleaner than interpolation. I could switch them to interpolation if you
> prefer that.

I don't have a strong preference I just thought the tests seemed inconsistent.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/pub_uploader_test.dart (right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/pub_uploader_test.dart:169: '{not json'));
On 2014/02/18 22:01:10, nweiz wrote:
> On 2014/02/14 17:55:02, Bob Nystrom wrote:
> > Be consistent about using emitsString() or multiple calls to .stderr.expect.
> > (I'd just use multiple calls.)
> 
> I chose [emitsString] for consistency with longer multiline expectations,
where
> multiple calls to [expect] get very cumbersome. Previously we had been using
> multiple calls by necessity, since there was no facility for matching multiple
> lines in a single call, but now we do so I thought it would be cleaner to use
> it.

That makes sense to me too, but in that case I'd probably use emitsString() for
even the two line cases. I thought I saw a bunch that still used separate expect
calls for that.

But, again, I don't have a strong preference here, so don't spend too much time
on it.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/serve/warns_on_assets_paths_test.dart (right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve/warns_on_assets_paths_test.dart:20: r'^Warning:
Pub reserves paths containing "assets" for using assets from '
On 2014/02/18 22:01:10, nweiz wrote:
> On 2014/02/14 17:55:02, Bob Nystrom wrote:
> > Given that the second line intentionally isn't raw, why make this one?
> 
> I think RegExp strings should be raw wherever possible so people don't have to
> think about to escape a character or not every time they write a regexp. I'd
> almost rather the next line be
> 
>     r'packages\. Please rename the path "' + assetsPath + r'\.$'

Eh, that seems worse to me. I think it's up to the programmer to be aware of
what kind of string their modifying.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/test_pub.dart:446: "Looks great! Are you ready to
upload your package (y/n)?"));
On 2014/02/18 22:01:10, nweiz wrote:
> On 2014/02/14 17:55:02, Bob Nystrom wrote:
> > Multi-line string?
> 
> Same response as before -- why here and not elsewhere?

Same response as before. It's already six lines of strings, may as well make a
six-line string and get rid of the explicit "\n"s.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/test_pub.dart:871: StreamMatcher emitsString(String
output) => inOrder(output.split("\n"));
On 2014/02/18 22:01:10, nweiz wrote:
> On 2014/02/14 17:55:02, Bob Nystrom wrote:
> > How about "emitsLines"?
> 
> I considered that name, but it felt weird that it took a string rather than
> lines. I can change it if emitsLines feels more sensible to you, though.

Yeah, I think it does. Before I saw the doc comment here, I didn't realize that
it actually did line splitting, which I think is the most important bit of
semantics for the function. I think the name should stress that.

[nweiz, 2014-02-18 22:28:01]

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/lish/preview_package_validation_has_a_warning_test.dart
(right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/lish/preview_package_validation_has_a_warning_test.dart:31:
'\n'
On 2014/02/18 22:12:59, Bob Nystrom wrote:
> On 2014/02/18 22:01:10, nweiz wrote:
> > On 2014/02/14 17:55:02, Bob Nystrom wrote:
> > > It might be cleaner just to use a multi-line string here.
> > 
> > That's contrary to how we usually do long multiline strings.
> 
> Is that true? We have a decent number of tests with multiline strings.

That's true, but they tend to be older tests or whitespace-insensitive tests,
and almost none of our actual library code uses multiline strings. As I recall,
you wanted to avoid using multiline strings where whitespace was important
because the indentation wouldn't match up.

I don't have a strong preference between the two, but I do think we should be
consistent between multiple tests and between tests and library code. I also
don't like the idea of using a re-indent function all over the place.

> > Why do you think a multiline string would be a good fit here and not
> elsewhere?
> 
> Here you have a string that's four lines long and has a bunch of explicit
> newlines. Since the string literal itself is already spread across multiple
> lines, you may as well use that to your advantage and use the implicit
newlines
> that a multiline string gives you.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/serve/warns_on_assets_paths_test.dart (right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve/warns_on_assets_paths_test.dart:20: r'^Warning:
Pub reserves paths containing "assets" for using assets from '
On 2014/02/18 22:12:59, Bob Nystrom wrote:
> On 2014/02/18 22:01:10, nweiz wrote:
> > On 2014/02/14 17:55:02, Bob Nystrom wrote:
> > > Given that the second line intentionally isn't raw, why make this one?
> > 
> > I think RegExp strings should be raw wherever possible so people don't have
to
> > think about to escape a character or not every time they write a regexp. I'd
> > almost rather the next line be
> > 
> >     r'packages\. Please rename the path "' + assetsPath + r'\.$'
> 
> Eh, that seems worse to me. I think it's up to the programmer to be aware of
> what kind of string their modifying.

It's always up to the programmer to be aware of what they're doing, but that
doesn't mean we shouldn't try to mitigate the cognitive overhead where possible.
We're already pretty consistent about this, and we still have occasional bugs
with improper regexp escaping.

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/test_pub.dart:871: StreamMatcher emitsString(String
output) => inOrder(output.split("\n"));
On 2014/02/18 22:12:59, Bob Nystrom wrote:
> On 2014/02/18 22:01:10, nweiz wrote:
> > On 2014/02/14 17:55:02, Bob Nystrom wrote:
> > > How about "emitsLines"?
> > 
> > I considered that name, but it felt weird that it took a string rather than
> > lines. I can change it if emitsLines feels more sensible to you, though.
> 
> Yeah, I think it does. Before I saw the doc comment here, I didn't realize
that
> it actually did line splitting, which I think is the most important bit of
> semantics for the function. I think the name should stress that.

Done.

[Bob Nystrom, 2014-02-18 22:48:22]

LGTM!

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/lish/preview_package_validation_has_a_warning_test.dart
(right):

https://codereview.chromium.org/164773003/diff/30001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/lish/preview_package_validation_has_a_warning_test.dart:31:
'\n'
On 2014/02/18 22:28:02, nweiz wrote:
> On 2014/02/18 22:12:59, Bob Nystrom wrote:
> > On 2014/02/18 22:01:10, nweiz wrote:
> > > On 2014/02/14 17:55:02, Bob Nystrom wrote:
> > > > It might be cleaner just to use a multi-line string here.
> > > 
> > > That's contrary to how we usually do long multiline strings.
> > 
> > Is that true? We have a decent number of tests with multiline strings.
> 
> That's true, but they tend to be older tests or whitespace-insensitive tests,
> and almost none of our actual library code uses multiline strings. As I
recall,
> you wanted to avoid using multiline strings where whitespace was important
> because the indentation wouldn't match up.
> 
> I don't have a strong preference between the two, but I do think we should be
> consistent between multiple tests and between tests and library code. I also
> don't like the idea of using a re-indent function all over the place.

+1 for consistency. I'm OK with avoiding multiline strings if you want.

> 
> > > Why do you think a multiline string would be a good fit here and not
> > elsewhere?
> > 
> > Here you have a string that's four lines long and has a bunch of explicit
> > newlines. Since the string literal itself is already spread across multiple
> > lines, you may as well use that to your advantage and use the implicit
> newlines
> > that a multiline string gives you.
>

[nweiz, 2014-02-18 22:57:20]

Committed patchset #4 manually as r32770 (presubmit successful).