Skip to content

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Sep 21, 2020

Description

Detect when an exit()s and don't pass on further logging to the log reader. This pretty much never happens on iOS apps because the App Store will reject apps that exit themselves manually, but I did see it while I was writing integration tests.

Related Issues

Follow up on #66092.

Tests

  • Unit test for process exit log line.
  • Convert flutter_run_test to an integration logging test (not just adb logging). Run on iOS, let's stop regressing logging on that platform...

Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.

@jmagman jmagman added c: contributor-productivity Team-specific productivity, code health, technical debt. platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. labels Sep 21, 2020
@jmagman jmagman self-assigned this Sep 21, 2020
run.kill();
});
return passedTest && failedTest && skippedTest && finishedMessage
return passedTest && failedTest && skippedTest && finishedMessage && printMessage && writelnMessage
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails on stable (neither line is printed).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

..listen(receivedLogLines.add);

expect(await iosDeployDebugger.launchAndAttach(), isTrue);
await logLines.toList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could also await logLines.last

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it doesn't like that:

dart:async                                          Stream.last
test/general.shard/ios/ios_deploy_test.dart 117:24  main.<fn>.<fn>.<fn>
===== asynchronous gap ===========================
dart:async                                          _asyncThenWrapperHelper
test/general.shard/ios/ios_deploy_test.dart         main.<fn>.<fn>.<fn>
dart:async                                          runZoned
test/src/common.dart 207:14                         testWithoutContext.<fn>
test/src/common.dart 177:18                         test.<fn>

Bad state: No element

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, because it is empty...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toList seems easier than Listen/onDone

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting the streams to work in these tests was 3x more work than the actual change.

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nit

@jmagman
Copy link
Member Author

jmagman commented Sep 22, 2020

Dependency #66092 was reverted.

@jmagman
Copy link
Member Author

jmagman commented Sep 23, 2020

Closing, this was included with #66399.

@jmagman jmagman closed this Sep 23, 2020
@jmagman jmagman deleted the process-exit-log branch September 23, 2020 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants