Skip to content

Add a E2E based performance test case #61509

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

Merged
merged 23 commits into from
Jul 23, 2020
Merged

Add a E2E based performance test case #61509

merged 23 commits into from
Jul 23, 2020

Conversation

CareF
Copy link
Contributor

@CareF CareF commented Jul 15, 2020

Description

This PR reimplement cull_opacity_perf__timeline_summary test as cull_opacity_perf__e2e_summary using E2E, as a trial for running performance test using E2E that's host-independent in terms of driving the test.

Related Issues

This is the first migration of performance test for go/flutter-nonhost
This is also a first step to #38329 for the new test case is self-driven.

Tests

The PR itself is adding a test case

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.
  • 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

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.

@CareF CareF self-assigned this Jul 15, 2020
@fluttergithubbot fluttergithubbot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Jul 15, 2020
@CareF CareF marked this pull request as draft July 15, 2020 02:29
@CareF CareF marked this pull request as ready for review July 16, 2020 03:09
@CareF CareF requested a review from liyuqian July 16, 2020 03:12
@CareF
Copy link
Contributor Author

CareF commented Jul 16, 2020

A few things to address:

  • part of dev/benchmarks/macrobenchmarks/test_driver/e2e_test.dart and dev/benchmarks/macrobenchmarks/test/util.dart may be better go to E2E;
  • Should we or when should we replace the original perf test case: For this one there will be duplicate on the flutter-build webpage, but I don't think eventually we need both flutter_driver version and E2E version running.

Before merge I need to run flutter update-packages --force-upgrade.

@CareF CareF requested a review from dnfield July 16, 2020 18:51
@CareF CareF mentioned this pull request Jul 16, 2020
9 tasks
@@ -0,0 +1,192 @@
// Copyright 2014 The Flutter Authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

This file probably also belongs to test_driver as this is unlikely to be used as a unit test. BTW, I guess we'll update WidgetTester to WidgetController in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not expecting to use that demo very soon but if you think I should, maybe I'd better change this PR back to draft as that demo is far from use-able.

Copy link
Contributor Author

@CareF CareF Jul 16, 2020

Choose a reason for hiding this comment

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

And the reason I'm putting it in test/ rather than test_driver is that E2E recommends so (https://pub.dev/packages/e2e#test-locations), and the test file (cull_opacity_perf_e2e.dart) should be able to run using flutter test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing from WidgetTester to WidgetController for driverOps and setupOps for now, but this specific case it's not used.


import 'util.dart';

Future<void> main() async {
Copy link
Contributor

Choose a reason for hiding this comment

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

This file probably belongs to test_driver as this is unlikely to be used as a unit test.

Copy link
Contributor Author

@CareF CareF Jul 16, 2020

Choose a reason for hiding this comment

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

Same reason above. #61509 (comment)

@CareF
Copy link
Contributor Author

CareF commented Jul 17, 2020

I may also need to update the macrobenchmark/readme.md, which is also the case for #61388. That will be done on whichever of the two PR who lands later.

Copy link
Contributor

@liyuqian liyuqian left a comment

Choose a reason for hiding this comment

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

I think this is good to go as soon as we minimize the duplicate code to make it a little easier to maintain in the future. Let's try to have this running in our device lab soon :)

Copy link
Contributor

@liyuqian liyuqian 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 nits. CC @dnfield to see if there's any extra comments.

_countExceed(frameRasterizerTimeSorted, kBuildBudget),
);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: when a function has a lot of arguments, Flutter usually put them under {} so it would be like the following

const FrameTimingSummarizer._({
  this.frameBuildTime,
  ...
});

// Use it with argument name provided to reduce confusion
FrameTimingSummarizer._(
  ...,
  averageFrameBuildTime: frameBuildTime.reduce(add) ~/ data.length,
  ...,
);

Copy link
Contributor Author

@CareF CareF Jul 21, 2020

Choose a reason for hiding this comment

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

{} means named arguments, and optional if not marked as @required, with default value if not assigned null. Here I don't think they should be optional. I added all @required.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants