Skip to content

Conversation

@jiahaog
Copy link
Member

@jiahaog jiahaog commented Jan 18, 2021

This is a refactoring to simplify how flutter test runs tests with FlutterTestPlatform.

Currently, the tester can crash before and after the test harness connects to the websocket, with the use of InitialResult. The control flow is nested, resulting in the rough state diagram below:

       Start tester
            |
   Observatory connected
    /                \
Websocket connected   Tester Process Crash (1)
    |
Connect to websocket,
run test 
    |       \
All ok     Tester Process Crash (2)

Instead, in this PR, we share the same crash state, which simplifies the logic and makes the workflow easier to reason about.

       Start tester
            |
   Observatory connected
    /                \
Websocket connected---\
    |                  \
Connect to websocket,---\
run test                 \
    |                     \
All ok                 Tester Process Crash 
                      

Other more minor changes:

  • Skip waiting for the observatory, and just wait for the remote webSocket connection because this is what is really needed
  • Currently, we don't call Watcher?.handleTestCrashed when the tester crashes after the harness connects to it, but we do call it when the tester crashes before the harness connects to it. This is fixed here.
  • We also don't call watcher?.handleStartedProcess(ProcessEvent(ourTestCount, process, processObservatoryUri)) when enableObservatory is false. The code path is only active when that is enabled, which is fixed here with the refactoring to use a helper method to deal with connecting to the observatory. Because of this, we need to fix the package:integration_test tests
  • Remove when from _getExitCodeMessage for simplicity, so that we don't have to track the state of the test purely for logging purposes. This doesn't seem useful because when is mainly an implementation detail. When troubleshooting (in verbose mode), the trace messages added would be much more useful in determining the state of the test, and some additional trace messages have been added.
  • Remove InitialResult, TestResult, introduce a private _TestHarnessStatus

Existing tests should cover this refactoring.

Related: #66264. This PR will make it easier to decouple FlutterPlatform from Process (#74236)

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jan 18, 2021
@google-cla google-cla bot added the cla: yes label Jan 18, 2021
@jiahaog jiahaog force-pushed the simplify-test branch 2 times, most recently from ed3964d to 559b875 Compare January 18, 2021 08:47
@jiahaog jiahaog marked this pull request as ready for review January 18, 2021 14:43
@jiahaog jiahaog requested a review from jonahwilliams January 18, 2021 14:43
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

I like the simplification here. I'm slightly concerned we're going to introduce a new edge case in a difficult to track way, but I think we can deal with that by holding this until we cut our next release. Sound good?

@jiahaog
Copy link
Member Author

jiahaog commented Jan 19, 2021

That sounds good. Does that mean that we should not merge this to master now? I'm not too familiar with the release process, do you know when will that be?

@jonahwilliams
Copy link
Contributor

Yes, please hold off on merging this. We're going to cut the next release in the next few days, after which point I will let you know that this is safe to land.

@jonahwilliams
Copy link
Contributor

LGTM, this is good to land

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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