chore(#9483): initial setup of apdex automation in cht-core #9037
Conversation
tests/performance/apdex-score/test/pageobjects/messages.page.js
Outdated
Show resolved
Hide resolved
tests/performance/apdex-score/test/pageobjects/performance.page.js
Outdated
Show resolved
Hide resolved
|
@lorerod kindly have a second look at this - thanks |
There was a problem hiding this comment.
Great start!!
Most of my comments are about the code being extremely bespoke, a high amount of duplicate code and slight inconsistencies in selectors, and the liberal use of browser.pause.
I think I'm recommending to:
- use abstraction layers more to generalize selectors - like moving generic selectors in page.js.
- reduce the use of hardcoded strings, especially in config-based differentiation. I think the best would be to group the strings somewhere and access them somehow.
- compact code so we don't have a 1200 line file where more than half are one-line functions. This is very hard to read and review and maintain. Bespoke functions are useful when there is high external reuse. I'm not sure that every one of functions will have such high external reuse to warrant the space it takes.
- remove pauses, instead figure out what we're waiting for, there is a high change that we can detect if the thing happened from something changes on the screen, and wait for that instead of using a generic
pause.
I really like the class based approach, I think this extends really well into my request for abstracting selectors and it looks really clean. Great job there!
|
@dianabarsan & @lorerod Thanks for the review and the comments. We intend to make incremental changes by merging smaller PR's to this branch before submitting a final version for review and I'm hoping that all these suggestions/changes requested will be addressed one after the other. |
) Co-authored-by: Rafa <[email protected]>
… the settings file (#9144) Co-authored-by: Rafa <[email protected]>
|
Hi @dianabarsan @lorerod All RFC's have been resolved - I'm going ahead to make this a draft for now as we are still making incremental changes to this work from other branches. |
|
@ralfudx, this PR hasn't had any updates for two weeks. |
|
@latin-panda I've just created this task as part of this week's commitments to address this - as previously stated we'll need to merge the modification to capture tasks:refresh telemetry data, clean up the code and then resubmit the PR for review - in the middle of all this, we will also enrich/update the documentation on how to setup as you've recommended. |
ralfudx
changed the title
|
@dianabarsan @lorerod @tatilepizs Thanks for looking at this and dropping your comments. I have implemented the requested changes and this is now ready for another review |
|
@dianabarsan @lorerod @tatilepizs kindly have a look when you have some time |
There was a problem hiding this comment.
Looks good to me @ralfudx, than you for all the improvements ⭐
I was just wondering, if you are going to create a separate PR for cht-docs with the all the information maybe you can remove the README.md so you don't have to create another PR in a few days just to remove it 😊
Yes @tatilepizs I'll be making a separate PR for the cht-docs (since it's in a different repo) - also I'll be adding another sample settings file for |
|
@dianabarsan I made the requested changes - kindly have a look again thanks |
Benmuiruri
changed the title
Description
closes #9483
Code review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.