Properly pass environment variables from pub to dart2js.

Properly pass environment variables from pub to dart2js.

R=rnystrom@google.com
BUG=16298
Committed: https://code.google.com/p/dart/source/detail?r=32144

[Bob Nystrom, 2014-01-29 17:08:24]

I think we could use some better test coverage. I also stumbled onto the issue
yesterday that configuring dart2js breaks pub build (I have a fix and test for
that), so I feel like we're lacking a bit here.

Aside from removing the comment about this being a regression, this patch LGTM!

https://codereview.chromium.org/136003018/diff/40001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/transformer/dart2js/passes_along_environment_constants_test.dart
(right):

https://codereview.chromium.org/136003018/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/transformer/dart2js/passes_along_environment_constants_test.dart:13:
// This is a regression test for issue 16298.
This isn't really a regression test. As far as I can tell, we didn't have any
tests that the environment variables were actually passed in correctly, just
that dart2js didn't barf when we did.

It would be good to have some tests that the other options do what we expect
too, at least in case where it can be tested (like minify).

[nweiz, 2014-01-29 20:39:44]

> I think we could use some better test coverage. I also stumbled onto the issue
> yesterday that configuring dart2js breaks pub build (I have a fix and test for
> that), so I feel like we're lacking a bit here.

In general it's not feasible to test the all combinations of features. It was
reasonable to believe that dart2js configuration was orthogonal to "pub build";
I don't think that was a failure of test coverage.

https://codereview.chromium.org/136003018/diff/40001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/transformer/dart2js/passes_along_environment_constants_test.dart
(right):

https://codereview.chromium.org/136003018/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/transformer/dart2js/passes_along_environment_constants_test.dart:13:
// This is a regression test for issue 16298.
On 2014/01/29 17:08:24, Bob Nystrom wrote:
> This isn't really a regression test. As far as I can tell, we didn't have any
> tests that the environment variables were actually passed in correctly, just
> that dart2js didn't barf when we did.

Done.

> It would be good to have some tests that the other options do what we expect
> too, at least in case where it can be tested (like minify).

There's already a test for minify.

[Bob Nystrom, 2014-01-29 21:21:25]

On 2014/01/29 20:39:44, nweiz wrote:
> > I think we could use some better test coverage. I also stumbled onto the
issue
> > yesterday that configuring dart2js breaks pub build (I have a fix and test
for
> > that), so I feel like we're lacking a bit here.
> 
> In general it's not feasible to test the all combinations of features.

Sure, but we do test quite a few combinations. For example, we run lots of tests
of identical behavior in both pub get and pub upgrade.

> It was reasonable to believe that dart2js configuration was orthogonal to "pub
build";

Yeah, I don't think you should have *known* this would be broken. It should have
been orthogonal, and the code in build.dart for dealing with the dart2js
transformer was hacky. You shouldn't feel bad.

> I don't think that was a failure of test coverage.

Well, right now (or at least before my patch went in), if you configured dart2js
at all and used the pub build command (which is precisely the thing you want to
configure!) it would crash pub. The ability to configure dart2js at all was a
completely broken feature. That seems like something we'd want a test to catch.
Certainly, if we break that *again* we'd want to know.

> > It would be good to have some tests that the other options do what we expect
> > too, at least in case where it can be tested (like minify).
> 
> There's already a test for minify.

OK, can we test any of the other options? I thought we tended to be pretty
scrupulous about testing things.

- bob

[nweiz, 2014-01-29 22:08:12]

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