Skip to content

LibTest/core/String/String.fromEnvironment_A01_t01 should expect the empty string #540

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

Closed
eernstg opened this issue Mar 10, 2020 · 5 comments
Assignees
Labels
type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@eernstg
Copy link
Member

eernstg commented Mar 10, 2020

Said test expects new String.fromEnvironment(v) to evaluate to null when v is a string which is not defined in the environment.

However, with the upcoming changes here, it will evaluate to the empty string rather than null, so the test needs to be updated to expect that.

This change is being backported, so the change needs to be made in the pre-nnbd branch, but it will also affect the master branch version, so both tests need to be updated.

@eernstg eernstg added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Mar 10, 2020
@eernstg
Copy link
Member Author

eernstg commented Mar 10, 2020

Note that the changes being backported have not all been landed, so it may be a good idea to check the corresponding CL in order to see that the tools are actually in a state where the change has taken place.

@eernstg
Copy link
Member Author

eernstg commented Mar 12, 2020

A similar change is needed in co19_2/LibTest/core/int/int.fromEnvironment_A01_t01.dart, as well as in the corresponding test from the master branch. It might be useful to add a test case where a defaultValue is passed, making this test similar to the one for String which is mentioned above.

@eernstg
Copy link
Member Author

eernstg commented Mar 24, 2020

Here is the CL where the breaking change is performed: https://dart-review.googlesource.com/c/sdk/+/140640. So when that change lands, the above changes to the tests are needed.

@eernstg
Copy link
Member Author

eernstg commented Mar 25, 2020

Said CL is now landing, so the changes are needed now.

@sgrekhov
Copy link
Contributor

Fixed in both branches

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

2 participants