Bugfixes for DevTools package

[christian-bromann]

Proposed changes

This patch has fixes for #4443, #4447 and #4448.

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)

Further comments

Not quite done yet, have to check how I can incorporate tests for these fixes.

Reviewers: @webdriverio/project-committers

[codecov]

Codecov Report

Merging #4459 into master will increase coverage by <.01%.
The diff coverage is 96.87%.

@@            Coverage Diff             @@
##           master    #4459      +/-   ##
==========================================
+ Coverage   99.25%   99.26%   +<.01%     
==========================================
  Files         188      188              
  Lines        4725     4754      +29     
  Branches     1010     1015       +5     
==========================================
+ Hits         4690     4719      +29     
  Misses         32       32              
  Partials        3        3

Impacted Files	Coverage Δ
...erio/src/commands/element/isDisplayedInViewport.js	100% <ø> (+33.33%)	⬆️
packages/devtools/src/utils.js	98.73% <ø> (ø)	⬆️
packages/devtools/src/devtoolsdriver.js	98.68% <96.87%> (-1.32%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cecf11b...1aca9e1. Read the comment docs.

[christian-bromann]

So I think I added necessary unit tests, this is ready for review.

[christian-bromann]

@mgrybyk I removed the isDisplayed capability from the DevTools package. I think it is difficult to define what visible means which is why neither Puppeteer nor WebDriver has command for it. That said, in order to have a single source of truth let's have them both execute the JS code we have to determine visibility for both devtools and webdriver

[mgrybyk]

Add some comments, I think we should be more strict when verifying page titles to make sure that navigation has actually happened when expected

[christian-bromann]

I think we should be more strict when verifying page titles

No because this is not part of the protocol, technically if you click on a link in WebDriver and get the title, chances are that you still get the old title. Therefor it is just important for me here, that we get a result

[christian-bromann]

This will also fix the flaky tests on master

[mgrybyk]

Why then with Webdriver the test always passing (waiting for page to load?) when click triggers navigation while with devtools protocol it always fail?

[christian-bromann]

when click triggers navigation while with devtools protocol it always fail?

Do you have a reproducible example?

[mgrybyk]

@christian-bromann
I know it's getUrl instead of getTitle, is it same issue?

const assert = require('assert')

describe("remote driver", () => {
    it("foobar", () => {
        browser.url("http://guinea-pig.webdriver.io")

        $("#githubRepo").click()
        const url = browser.getUrl()
        assert.strictEqual(url, 'https://github.com/')
    })
})

always fails in devtools
never fails in webdriver

[mgrybyk]

this one #4447 is failing now because isElementDisplayed is not implemented when doing $('#foobarTest').waitForDisplayed(4000)

[christian-bromann]

I increased the timeout for frame navigation and updated the e2e test to actually check the title. What do you think?

[mgrybyk]

Taking a look.
In a meanwhile isDisplayed also have to be fixed

[mgrybyk]

@christian-bromann to be honest I don't like it, now every click takes one second, also not only click can lead to site loading, ex: it can be any javascript call.

What if we fully remove everything related to site load from click and implement something like webdriver's page load strategy?

As far as I've understood if page is loading next command call is waiting for page to load. Is it possible to implement something similar with puppeteer?

before executing any command verify if document.readyState is not in ['interactive', 'complete'] and only then proceed otherwise wait for load or domcontentloaded page event.

[christian-bromann]

@mgrybyk I agree with you, I wasn't really happy with it either. I check the Puppeteer API and found that they have an ExecutionContext class and it can be used to check before every command if and execution context exists. Unfortunately that doesn't help completely to avoid issues especially when a transition happens randomly which is why I kept the fallback where we end up re-executing the command. What do you think?

[mgrybyk]

It is better now, thank you! However I still have to wait for page to load manually and I think it can be improved.

Here is what I've figured out while debugging:

// add listeners for debugging in devtoolsdriver.js
// page.on('load', () => console.log('EVENT: page load'));
// page.on('domcontentloaded', () => console.log('EVENT: page domcontentloaded'));
// page.on('framenavigated', () => console.log('EVENT: page framenavigated'));

// open some long loading link (with click or javascript)
browser.execute(function () {
                this.document.location = 'https://secure.gamblingcommission.gov.uk/PublicRegister/Search/Detail/50970'
})

browser.waitUntil(() => {
    const readyState: string = browser.execute('return document.readyState')
    console.log('readyState', readyState)
    return ['complete', 'interactive'].includes(readyState)
}, 15000, 'oh snap', 300)

browser.pause(2000)
console.log('readyState', browser.execute('return document.readyState'))

Here is an output:

[0-0] EVENT: page framenavigated
[0-0] readyState loading // have to wait manually
[0-0] readyState loading
[0-0] readyState loading
[0-0] EVENT: page domcontentloaded // same as readyState interactive?
[0-0] readyState interactive  // even though site has loaded some JS is executing
[0-0] EVENT: page load // same as readyState complete?
[0-0] readyState complete // now I can do everything

What I wanted to say, do you think it makes sense to wait for document.readyState to be interactive at least?
We may allow user to configure page load strategy in similar way to webdriver' drivers (eager, normal, none) https://stackoverflow.com/a/43737358/2475987

It may be done with another PR later on but I think it's quite useful.

[christian-bromann]

Good call @mgrybyk, added this to the PR, please have a look

[mgrybyk]

Hey, I'll review later today.

[mgrybyk]

@christian-bromann I'm getting page.mainFrame is not a function
for line const executionContext = await page.mainFrame().executionContext()

Looking for steps to reproduce

[mgrybyk]

@christian-bromann

it("foobar", () => {
    browser.url("https://fiddle.jshell.net/westonruter/6mSuK/show/")
    $("#result iframe").waitForDisplayed()
    browser.pause(1000)
    assert.strictEqual(($("iframe").getAttribute("src")).includes("fiddle"), true)
    browser.switchToFrame($("#result iframe"))
})

Execution fails, browser stays opened.
Log:

ERROR @wdio/local-runner: Failed launching test session: TypeError: page.mainFrame is not a function
    at DevToolsDriver.checkPendingNavigations (/e2e/node_modules/devtools/build/devtoolsdriver.js:152:41)
    at Browser.wrappedCommand (/e2e/node_modules/devtools/build/devtoolsdriver.js:79:18)
    at Runner.endSession (/e2e/node_modules/@wdio/runner/build/index.js:241:26)
    at Runner.run (/e2e/node_modules/@wdio/runner/build/index.js:135:18)

[christian-bromann]

@mgrybyk this should be fixed with #4444 which is why I rebased the branch

[christian-bromann]

nevermind, I see the error in the build .. taking a look

[christian-bromann]

Added a fix for it

[mgrybyk]

I don't have any other comments for this PR except of adding e2e tests for switching to iframe that can be done separately later on.
Verified cases from #4443, #4447, #4448

Thank you!