-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
(webdriverio): add throttleCPU and throttleNetwork commands #11548
(webdriverio): add throttleCPU and throttleNetwork commands #11548
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments but overall looks good.
Thanks @christian-bromann I have resolved your feedback 👍 |
Is there a way to improve the failing test? I don't see why we check the amount of browser commands |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to see a better deprecation system because this message gets lost in the logs and I wonder if we can better amplify it in the stdout. However this should not block this PR to get merged.
LGTM 👍
Can this be merged with the failing tests? |
I think we should fix the one that is failing. I can take a look when I have time. |
These are the same tests that are failing for #11542 and I honestly have no idea why it's failing in either of these PR's 😮 |
Not quite, in this PR this test is failing:
in #11542 the following tests are failing:
For latter you have to adjust the assertions since you now set different arguments when starting a Chromedriver session. For this PR it seems like the throttle command is not attached to the scope properly. |
@@ -29,7 +29,8 @@ export * from './browser/setCookies.js' | |||
export * from './browser/setTimeout.js' | |||
export * from './browser/setWindowSize.js' | |||
export * from './browser/switchWindow.js' | |||
export * from './browser/throttle.js' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By removing this export, the command is not attached to the browser scope anymore which would cause a breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolve build issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will go ahead and merge this and gonna fix remaining test issues in main
.
Thanks a lot, this is awesome 👍
Proposed changes
This allows our users to easily throttle both and debug flaky tests.
We can remove the throttle command itself in v9.
Types of changes
Checklist
Reviewers: @webdriverio/project-committers