test(button-harness): add performance tests for buttons using the pro… #20222

wagnermaciel · 2020-08-06T19:40:22Z

…tractor harness env

The main thing we are looking at here is not the button's performance. Instead we are looking at
the performance overhead of using the protractor harness env.

test/benchmarks/material/button-harness/app.module.ts

test/benchmarks/material/button-harness/protractor.perf-spec.ts

mmalerba

LGTM

devversion

First drive-by review. Will have a more closer look as soon as possible.

test/benchmarks/material/button-harness/protractor.perf-spec.ts

devversion · 2020-08-14T20:12:14Z

test/benchmarks/material/button-harness/protractor-benchmark-utilities.ts

+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {runBenchmark} from '@angular/dev-infra-private/benchmark/driver-utilities';


Are these utilities specific to button? Would it make more sense to put this into something higher, or even in dev-infra? At least as part of this PR it could be moved into test/benchmarks/utils?

Hmmm, I'm not sure. I created the protractor and testbed benchmark utils so that their tests would be cleaner & simpler.

The testbed one uses console.warn which no other test is gonna want to use. The protractor one is more likely to be reusable since we might actually want to get the full time of a test instead of getting the sub-metrics returned by benchpress

I also think I need to go back and try to set up default params for runBenchmark again. I tried to do it a while back and surprisingly had a bunch of weird typescript errors throw

Do you think it would be sufficient to try and simplify the runBenchmark params instead?

Yeah, I think simplifying runBenchmark is what we want. I asked in the past for making ignoreBrowserSynchronization the default. I'd love us simplifying this.

Regardless of the runBenchmark API being simpler though; I think it might be good to move these shorthands (for using console.warn etc) into a higher-level because I assume we'll have more harness benchmarks, right?

I believe @mmalerba only needs this harness benchmark

Fair enough. I didn't know this is the case. They sounded quite generic.

Yeah these are not intended as performance tests for the button harness, they're intended as tests for the harness framework itself, they just happen to be using button to test that. I actually think we should change the directory name to make that more clear. How about test/benchmarks/cdk/testing/*?

That now makes sense 😄 thanks Miles for clarifying!

test/benchmarks/material/button-harness/app.module.ts

wagnermaciel · 2020-08-14T20:24:11Z

@devversion Sorry, I should have left a comment. I pushed these changes out so that Miles could take a look. None of these changes are concrete right now

devversion · 2020-08-14T20:26:54Z

@wagnermaciel Oh, sorry! 😄 I got requested for review and looked since I didn't want people to be blocked on me 😛 Let me know if you want me to have a full look when it's done.

wagnermaciel · 2020-08-17T23:38:54Z

@devversion Ok, ready for review. My bad earlier, I should have let you know. And thanks for being so fast to review! I really value & appreciate your feedback!

devversion · 2020-08-20T12:00:55Z

test/benchmarks/material/button-harness/protractor-benchmark-utilities.ts

+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {runBenchmark} from '@angular/dev-infra-private/benchmark/driver-utilities';


Yeah, I think simplifying runBenchmark is what we want. I asked in the past for making ignoreBrowserSynchronization the default. I'd love us simplifying this.

devversion · 2020-08-20T12:01:29Z

test/benchmarks/material/button-harness/protractor-benchmark-utilities.ts

+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {runBenchmark} from '@angular/dev-infra-private/benchmark/driver-utilities';


Regardless of the runBenchmark API being simpler though; I think it might be good to move these shorthands (for using console.warn etc) into a higher-level because I assume we'll have more harness benchmarks, right?

test/benchmarks/material/button-harness/protractor-benchmark-utilities.ts

test/benchmarks/material/button-harness/protractor.perf-spec.ts

test/benchmarks/material/button-harness/BUILD.bazel

…tractor harness env * The main thing we are looking at here is not the button's performance. Instead we are looking at the performance overhead of using the protractor and testbed harness env. * We are running tests with & without the ProtractorHarnessEnvironment and with & without the TestbedHarnessEnvironment. * We are benchmarking 5 operations with a varying number of buttons: 1. Get the first button 2. Click the first button 3. Click the middle button 4. Click the last button 5. Click every button

…the protractor harness env

wagnermaciel

@devversion I addressed most of your comments but I had trouble reconciling some of them with these changes requested by Miles (in particular, the naming of the tests)

mmalerba@eb8d037

test/benchmarks/material/button-harness/BUILD.bazel

wagnermaciel · 2020-08-31T21:27:21Z

test/benchmarks/material/button-harness/protractor-benchmark-utilities.ts

+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {runBenchmark} from '@angular/dev-infra-private/benchmark/driver-utilities';


I believe @mmalerba only needs this harness benchmark

test/benchmarks/material/button-harness/constants.ts

test/benchmarks/material/button-harness/protractor.perf-spec.ts

wagnermaciel · 2020-09-01T18:53:36Z

test/benchmarks/material/button-harness/protractor.perf-spec.ts

+
+  it('should click the first button', async () => {
+    await benchmark('click first button', async () => {
+      const button = await loader.getHarness(MatButtonHarness.with({text: FIRST_BUTTON}));


I'm going to address this along with the previous comment in my next commit (in this same PR)

I want to get the bulk of the changes in first before I work on consolidating the two tests into one shared spec, and I'll be making this fix as part of that change.

mmalerba

Mostly looks good, I think we just need to change the directory name and re-work the protractor tests to look like the testbed ones

mmalerba · 2020-09-03T22:22:24Z

test/benchmarks/material/button-harness/protractor-benchmark-utilities.ts

+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {runBenchmark} from '@angular/dev-infra-private/benchmark/driver-utilities';


Yeah these are not intended as performance tests for the button harness, they're intended as tests for the harness framework itself, they just happen to be using button to test that. I actually think we should change the directory name to make that more clear. How about test/benchmarks/cdk/testing/*?

devversion

LGTM. Just the minor thinks Miles mentioned in his comment.

#20222) * test(button-harness): add performance tests for buttons using the protractor harness env * The main thing we are looking at here is not the button's performance. Instead we are looking at the performance overhead of using the protractor and testbed harness env. * We are running tests with & without the ProtractorHarnessEnvironment and with & without the TestbedHarnessEnvironment. * We are benchmarking 5 operations with a varying number of buttons: 1. Get the first button 2. Click the first button 3. Click the middle button 4. Click the last button 5. Click every button * fixup! test(button-harness): add performance tests for buttons using the protractor harness env * fixup! test(button-harness): add performance tests for buttons using the protractor harness env * fix(harness-benchmarks): lots of small changes * fixup! fix(harness-benchmarks): lots of small changes * refactor(harness-benchmarks): use hrtime instead of console.time * move test to test/benchmarks/cdk/testing * fix comment (cherry picked from commit 7177f3f)

angular-automatic-lock-bot · 2020-10-12T00:11:34Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

wagnermaciel requested a review from a team as a code owner August 6, 2020 19:40

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 6, 2020

wagnermaciel requested a review from mmalerba August 6, 2020 19:40

mmalerba reviewed Aug 6, 2020

View reviewed changes

test/benchmarks/material/button-harness/app.module.ts Outdated Show resolved Hide resolved

test/benchmarks/material/button-harness/protractor.perf-spec.ts Outdated Show resolved Hide resolved

mmalerba reviewed Aug 6, 2020

View reviewed changes

test/benchmarks/material/button-harness/protractor.perf-spec.ts Outdated Show resolved Hide resolved

mmalerba approved these changes Aug 7, 2020

View reviewed changes

mmalerba added action: merge The PR is ready for merge by the caretaker merge safe P2 The issue is important to a large percentage of users, with a workaround labels Aug 7, 2020

wagnermaciel removed the action: merge The PR is ready for merge by the caretaker label Aug 10, 2020

wagnermaciel requested a review from devversion as a code owner August 14, 2020 19:09

wagnermaciel removed P2 The issue is important to a large percentage of users, with a workaround merge safe labels Aug 14, 2020

devversion reviewed Aug 14, 2020

View reviewed changes

wagnermaciel force-pushed the protractor-harness-benchmark branch from 1fa517f to 72a4ae0 Compare August 17, 2020 21:33

wagnermaciel requested a review from devversion August 19, 2020 20:11

devversion reviewed Aug 20, 2020

View reviewed changes

wagnermaciel added 3 commits August 31, 2020 12:14

fixup! test(button-harness): add performance tests for buttons using …

14ff066

…the protractor harness env

fixup! test(button-harness): add performance tests for buttons using …

4dc2d68

…the protractor harness env

wagnermaciel commented Sep 1, 2020

View reviewed changes

wagnermaciel force-pushed the protractor-harness-benchmark branch from bf160e8 to f847ab5 Compare September 1, 2020 19:04

fix(harness-benchmarks): lots of small changes

6cbd4d1

wagnermaciel force-pushed the protractor-harness-benchmark branch from f847ab5 to 6cbd4d1 Compare September 1, 2020 19:05

wagnermaciel added 2 commits September 1, 2020 12:26

fixup! fix(harness-benchmarks): lots of small changes

f34e103

refactor(harness-benchmarks): use hrtime instead of console.time

2ea306f

mmalerba reviewed Sep 3, 2020

View reviewed changes

devversion approved these changes Sep 4, 2020

View reviewed changes

wagnermaciel added 2 commits September 8, 2020 07:28

move test to test/benchmarks/cdk/testing

a8ad042

fix comment

a589731

wagnermaciel added action: merge The PR is ready for merge by the caretaker merge safe target: patch This PR is targeted for the next patch release labels Sep 8, 2020

andrewseguin merged commit 7177f3f into angular:master Sep 11, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 12, 2020

wagnermaciel deleted the protractor-harness-benchmark branch January 14, 2021 19:01

test(button-harness): add performance tests for buttons using the pro… #20222

test(button-harness): add performance tests for buttons using the pro… #20222

Uh oh!

Conversation

wagnermaciel commented Aug 6, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mmalerba left a comment

Choose a reason for hiding this comment

Uh oh!

devversion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wagnermaciel commented Aug 14, 2020

Uh oh!

devversion commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wagnermaciel commented Aug 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wagnermaciel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmalerba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Oct 12, 2020

Uh oh!

Uh oh!

devversion commented Aug 14, 2020 •

edited

Loading