Skip to content

devtools commands for multiremote#11059

Merged
christian-bromann merged 1 commit intowebdriverio:mainfrom
ccharnkij:devtools-multiremote
Oct 2, 2023
Merged

devtools commands for multiremote#11059
christian-bromann merged 1 commit intowebdriverio:mainfrom
ccharnkij:devtools-multiremote

Conversation

@ccharnkij
Copy link
Contributor

@ccharnkij ccharnkij commented Aug 31, 2023

Proposed changes

Continuation from #5505, this feature allows devtools commands to be used for multiremote. The commands can be used with browser object itself, which produces array result for all instances, or with an individual instance.

// example cap
cap = 
{
  chrome1: {
    capabilities: {
      browserName: 'chrome'
    }
  },
 chrome2: {
    capabilities: {
      browserName: 'chrome'
    }
  },
}

await browser.cdp('Network', 'getCookies') // returns an array containing result for both instance
await chrome1.cdp('Network', 'getCookies')  // returns only result for chrome1

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have added proper type definitions for new commands (if appropriate)

Further comments

This change is pretty much done by moving the code from devtools/index.ts to devtools/commands.ts and having a separate command object for each browser instance. All commands can be used by an individual instance. However, because I try to do this while wanting to make as little changes as possible, not all commands can be used by browser and those command are: getNodeId , getNodeIds, startTracing, endTracing, getTraceLogs, and getPageWeight (the commands that originally exist in commands.ts file). If needed, I can make more change to allow those commands to be used by browser.

Also, performance testing with lighthouse is a little bit tricky to use. Because of the way beforeCommand work, I don't have a way to determine which instance the command is coming from, and this can cause the code to hang because onLoadEventFired and onFrameNavigated are not triggered.

// This works
await chrome1.enablePerformanceAudits()
await chrome1.url('https://webdriver.io')
await chrome1.disablePerformanceAudits()

// This will cause timeout
await chrome1.enablePerformanceAudits()
await chrome1.url('https://webdriver.io')
await chrome2.url('https://webdriver.io') // <---this line will causes chrome1 to hang because beforeCommand triggers 
                                       //  for both instances and chrome1 will get stuck waiting for onLoadEventFired to finish

So for performance testing, a user will have to do one instance at a time while having enablePerformanceAudits and disablePerformanceAudits before and after it.

Nothing change if a user is using normal mode.

Reviewers: @webdriverio/project-committers

@ccharnkij ccharnkij force-pushed the devtools-multiremote branch from a338c26 to 7ab406f Compare September 1, 2023 01:08
@christian-bromann christian-bromann self-requested a review September 1, 2023 15:26
@christian-bromann christian-bromann added the PR: New Feature 🚀 PRs that contain new features label Sep 1, 2023
@ccharnkij
Copy link
Contributor Author

Hi @christian-bromann, how has this been going?

@christian-bromann
Copy link
Member

@ccharnkij it seems like the coverage has dropped given all the changes, you think we can add some more unit tests to stay above treshold?

@ccharnkij ccharnkij force-pushed the devtools-multiremote branch 2 times, most recently from 8858777 to 7c88ac7 Compare October 1, 2023 02:02
@ccharnkij
Copy link
Contributor Author

ccharnkij commented Oct 1, 2023

@christian-bromann this one failed due to some ECONNRESET error. Is there any way I can rerun it w/o additional commit? I already added some tests and rebased it.

@ccharnkij ccharnkij force-pushed the devtools-multiremote branch from 7c88ac7 to a5151c4 Compare October 1, 2023 02:56
@ccharnkij
Copy link
Contributor Author

@christian-bromann nvm, looks like it goes well now.

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Great change set , nice work 👌

@christian-bromann christian-bromann merged commit a8ae7be into webdriverio:main Oct 2, 2023
@ccharnkij ccharnkij deleted the devtools-multiremote branch October 4, 2023 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: New Feature 🚀 PRs that contain new features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants