Need tests to work even with different defaultValue default values

[eernstg]

The test 'LibTest/core/String/String.fromEnvironment_A01_t01.dart' needs to be adjusted such that it will continue to work independently of the default value specified for defaultValue. So we need this:

var s2 = const bool.hasEnvironment("SOME_ENV_VARIABLE") ?
    const String.fromEnvironment("SOME_ENV_VARIABLE") : null;

Note that it should use const rather than new: The documentation of these constructors indicates that it is a dynamic error to invoke them at run time (so they can only be used in constant expressions). This is not implemented by all tools, but it is more important to make this test test something that should actually work, and then we may put a TODO somewhere to decide on whether to insist that the new usage should cause an exception, and write a separate test for that.

The test 'LibTest/core/int/int.fromEnvironment_A01_t01.dart' needs a corresponding update.

These changes need to be performed in both the master and the pre-nnbd branch, because the default values for defaultValue will be changed in both SDKs.

[sgrekhov]

Fixed in both branches

[eernstg]

Oops, please revert this and make the change as described in #540. It turns out that we have very few tests where we test that the default value for defaultValue is as expected for the fromEnvironment constructors, so it's better to keep the form that actually breaks when these default values differ from the expected value, rather than making the test independent of said default values.

But if it says new it should still be changed to const (because dart2js does actually implement the specified behavior which makes it a dynamic error to evaluate new String.fromEnvironment('...')).

Also note that the default values are the same both in master and pre-nnbd, because the backporting of the new default values has just landed today.

[sgrekhov]

I didn't rever but did the changes for #540 above this. Protection for the case when the system has an environment variable used in the test was left

[eernstg]

Sounds good! The important bit is that it should fail if the default value of defaultValue of int.fromEnvironment differs from 0, and for String.fromEnvironment if it differs from the empty string.

[eernstg]

[Coming back after a meeting, edited this whole comment.]

I can see that the new version of the test computes def1 to have the value "No" (assuming that this test will always be executed such that "SOME_ENV_VARIABLE" is not defined in the environment), and then we check that s1 gets the same value as the specified defaultValue. This was previously expressed in a more concise form that I believe would deliver the same amount of test coverage:

var s1 = const String.fromEnvironment('SOME_ENV_VARIABLE', defaultValue: 'No');
Expect.equals('No', s1);
var s2 = const String.fromEnvironment('SOME_ENV_VARIABLE');
Expect.equals('', s2);

(I fixed the news, they should be consts.) I think we should return to that form because it tests the default value of defaultValue directly.

The current form does not do that, because it uses bool.hasEnvironment to ensure that the default value of defaultValue is ignored.

The reason why this is important is that we have very few tests that actually test the default values of defaultValue, so it's a very concrete loss of coverage if we use hasEnvironment here.

Yes, I know: I asked for a rewrite of the kind that makes the test independent of these default values, but that was before I detected how tiny our test coverage is for the actual values.

[sgrekhov]

No problem. Changed in both branches