test(mdc-menu): add performance tests for mdc-menu #20494
Conversation
wagnermaciel
commented
Sep 3, 2020
wagnermaciel
commented
- Also cleaned up a small ts error I was getting from the material menu's driver.
googlebot
added
the
cla: yes
* Also cleaned up a small ts error I was getting from the material menu's driver.
wagnermaciel
force-pushed
the
mdc-menu-benchmark
branch
from
a54a7ed to
ab5cb65
Compare
| params: [], | ||
| setup: async () => { | ||
| trigger = element(by.buttonText('Basic Menu')); | ||
| }, |
There was a problem hiding this comment.
As a note for the dev-infra helper: We should remove the setup property from the runBenchmark function and just do the setup before. e.g. you could just directly assign the trigger variable based on what I can tell.
There was a problem hiding this comment.
You mean inside the work callback?
There was a problem hiding this comment.
No, I just meant before calling runBenchmark.
There was a problem hiding this comment.
The browser is not open until runBenchmark is called.
There was a problem hiding this comment.
IIRC the browser is always open with protractor, it just is on an empty page on initialization. So ideally IMO, the test would manually call await browser.open(<..>) before and then do all preparation work before starting with the samples.
The reason why I'm proposing this is that setup and prepare are very unclear and ambiguous IMO, and as it looks, it's also complicating the logic for preparations (as seen with above for finding the trigger). I don't feel very strongly about it. Changing the name of these properties might help too.
There was a problem hiding this comment.
Changing runBenchmark to not call openBrowser would require fixing all the old tests in angular/angular, too. Same with renaming setup, prepare, and work.
There was a problem hiding this comment.
Yeah, that's clear to me. Though I think that these usages should ideally not prevent us from maintaining good health and readable code. I mentioned this here as a side note, but I think we should do something in the future.
For the record: I'd be happy to go through the instances in the future if needed. We could also add a separate helper that has the less ambiguous API. All of that is obviously not a priority right now but something to keep in mind since we keep adding new benchmarks and spread this across repositories.
devversion
added
action: merge
* test(mdc-menu): add performance tests for mdc-menu * Also cleaned up a small ts error I was getting from the material menu's driver. * fix ts errors and reword test ids * change tabs to spaces (cherry picked from commit f298de3)
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
