Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[e2e] Add customizable flutter_driver adaptor #2859

Closed
wants to merge 9 commits into from

Conversation

CareF
Copy link
Contributor

@CareF CareF commented Jul 2, 2020

Description

Add a customizable flutter_driver adaptor, allowing changing timeout time, flag to enable timeline tracing.
A use case is here where I'm using the timeline to estimate overhead of E2E and flutter_driver (E2E is better!)

Related Issues

Not a full implementation but can be considered a work around for flutter/flutter#58789

Test

It seems that we don't yet have unit test for E2E. I changed the related example.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@CareF CareF requested a review from digiter as a code owner July 2, 2020 22:29
@CareF CareF self-assigned this Jul 2, 2020
@CareF CareF requested review from liyuqian and dnfield July 2, 2020 22:30
@CareF CareF force-pushed the e2e_customizable_driver_adaptor branch from cbe4b39 to 927cfe6 Compare July 6, 2020 15:45
@CareF CareF requested a review from dnfield July 6, 2020 15:46
@CareF CareF force-pushed the e2e_customizable_driver_adaptor branch 3 times, most recently from 392871b to 3a24002 Compare July 6, 2020 15:55
@dnfield
Copy link
Contributor

dnfield commented Jul 9, 2020

Looking at this a bit more, I think it would be better if we just offered some kind of API to get the timeline at various points, and to help with starting/stopping recording.

I don't think this is fine grained enough - someone may want the timeline for just part of their test, and they may not want to write the timeline to a local file system - they might need the test itself to send the timeline somewhere else.

Can we re-think the design here to handle that kind of use case?

@CareF
Copy link
Contributor Author

CareF commented Jul 9, 2020

Looking at this a bit more, I think it would be better if we just offered some kind of API to get the timeline at various points, and to help with starting/stopping recording.

I don't think this is fine grained enough - someone may want the timeline for just part of their test, and they may not want to write the timeline to a local file system - they might need the test itself to send the timeline somewhere else.

Can we re-think the design here to handle that kind of use case?

The hard part is, the test is driven on the device, and the timeline is collected on host. The generation of the timeline is distributed in framework and engine which we have very limited control. To achieve that there will have to be conversation between the device and the host, making the device side test code depending directly or indirectly on flutter_driver, and there will also be delay for this communication.
@dnfield

@dnfield
Copy link
Contributor

dnfield commented Jul 9, 2020

The timeline is actually connected on the device by the driver extension. It's fine to make it easier to shovel it over to the host to write, but we should expose an easier way to just collect it whether you end up using the driver extension or not.

@CareF
Copy link
Contributor Author

CareF commented Jul 9, 2020

So if the collecting and processing of timeline is completely on device, it makes finer graining but:

  1. It introduces the overhead of parsing json on device;
  2. We need to design a new set of APIs for triggering the timeline collecting, on the back of which we need more codes to connect E2E bindings to vmservice and flutter_driver.

I understand this PR doesn't provide a perfect solution, and that's why in the PR description I said

Not a full implementation but can be considered a work around for flutter/flutter#58789

The finer grain version should be do-able, but for my project I don't think I should invest more time on it because this feature is not critical for me. It will be great if this PR lands and I can use this feature for my project, otherwise I will put these codes separately in my project.

However I still feel this PR is useful. My example use case would be to measure the binding overhead. See this repo

Our (with @liyuqian) original plan for the performance test metrics is to implement a frame build time statistics API in or out of E2E, but independent of the Dart timeline, both for the purpose of host independency and for the purpose of using it in release mode.

@CareF CareF requested a review from dnfield July 10, 2020 17:04
@CareF
Copy link
Contributor Author

CareF commented Jul 14, 2020

@dnfield Would you mind letting me know if we finally decide to refuse this PR? I will need to add new features as flutter/flutter#61490 and I don't want to introduce conflicts.

@dnfield
Copy link
Contributor

dnfield commented Jul 15, 2020

I'd prefer to see a solution for this that does not require a host device to be attached. For example, we should support getting timeline information on Firebase Testlab.

@CareF
Copy link
Contributor Author

CareF commented Jul 15, 2020

I'd prefer to see a solution for this that does not require a host device to be attached. For example, we should support getting timeline information on Firebase Testlab.

@dnfield I think timeline requires vmservice, which I'm not sure how to have it enabled for an app running completely without a host.
And this PR is not about getting timeline within the app but getting timeline on the flutter_driver side of e2e.

@liyuqian
Copy link

https://github.com/flutter/flutter/pull/61509/files#diff-73e66281b14c792afcdcd20acb9edd92R5 says that this is blocking that PR. I wonder what's the future plan for this PR?

@CareF
Copy link
Contributor Author

CareF commented Jul 20, 2020

https://github.com/flutter/flutter/pull/61509/files#diff-73e66281b14c792afcdcd20acb9edd92R5 says that this is blocking that PR. I wonder what's the future plan for this PR?

@liyuqian I didn't update here because @dnfield mentioned:

I'd prefer to see a solution for this that does not require a host device to be attached. For example, we should support getting timeline information on Firebase Testlab.

I am not planning on implementing a host-independent timeline collection but #2873 would be an infra for that. It might make sense to modify what's here to make a new PR, with an e2e_driver that write the data defined in #2873 to a file, which will be used for flutter/flutter#61509.

@CareF CareF force-pushed the e2e_customizable_driver_adaptor branch from 21d5292 to 5ab1b59 Compare July 25, 2020 01:03
@CareF CareF force-pushed the e2e_customizable_driver_adaptor branch from 5ab1b59 to af57e0a Compare July 30, 2020 14:55
@CareF CareF closed this Aug 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants