-
Notifications
You must be signed in to change notification settings - Fork 27.2k
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
prevent accidental calls to io.exit when asserts are active. #46210
Conversation
Errm, right - assertions in integration tests. If we could detect that we are in a package:test isolate maybe this could be more narrowly focused. |
final ProcessResult result = await processManager.run(command, | ||
workingDirectory: folder, | ||
environment: <String, String>{ | ||
'FLUTTER_INTEGRATION_TEST': 'true' |
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.
In the previous two instances of this it's set to 'test' rather than 'true'.
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.
Fixed
/// Throws [StateError] if assertions are enabled and the dart:io exit | ||
/// is still active when called. This may indicate exit was called in | ||
/// a test without being configured correctly. This behavior can be | ||
/// removed by setting the `FLUTTER_INTEGRATION_TEST` environment |
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.
Is FLUTTER_INTEGRATION_TEST
new? Is there any pre-existing one that could work?
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.
there is a FLUTTER_TEST value that is set by flutter test
and for some reason by the test_driver.dart. I could reuse that field since I don't think it is used otherwise for pub run test
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.
Switched to FLUTTER_TEST
/// This is analogous to the `exit` function in `dart:io`, except that this | ||
/// function may be set to a testing-friendly value by calling | ||
/// [setExitFunctionForTests] (and then restored to its default implementation | ||
/// with [restoreExitFunction]). The default implementation delegates to | ||
/// `dart:io`. | ||
ExitFunction get exit => _exitFunction; | ||
ExitFunction get exit { | ||
assert(() { |
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.
Does this work?
assert(
_exitFunction != io.exit || io.Platform.environment['FLUTTER_TEST'] != null,
'io.exit was called with assertions active. If this is an integration test, '
'ensure that the environment variable FLUTTER_TEST is set '
'to a non-null String.',
);
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.
Good idea
Description
To prevent a scenario such as #46142 for occurring again in the future, prevent calls to dart:io's exit when asserts are enabled. Allow an env variable to override to allow for integration tests, which run the full flutter process with asserts enabled