Skip to content
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

chore(webkit): fix WebKit network-related driver tests #23232

Merged
merged 46 commits into from
Aug 18, 2022

Conversation

flotwig
Copy link
Contributor

@flotwig flotwig commented Aug 9, 2022

User facing changelog

n/a, WebKit is currently only available in development

Additional details

PR Tasks

  • Have tests been added/updated?
  • [na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [na] Has a PR for user-facing changes been opened in cypress-documentation?
  • [na] Have API changes been updated in the type definitions?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Aug 9, 2022

Thanks for taking the time to open a PR!

@flotwig flotwig mentioned this pull request Aug 9, 2022
21 tasks
@flotwig flotwig changed the title chore: fix WebKit pre-requests, XHR tests, proxy-logging, network stubbing chore(webkit): fix WebKit network-related driver tests Aug 15, 2022
Base automatically changed from cy-webkit to develop August 15, 2022 22:53
@@ -30,32 +31,7 @@ export async function open (browser: Browser, url, options: any = {}, automation
headless: browser.isHeadless,
})

let pwPage: playwright.Page

async function resetPage (_url) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lmiller1990 here is the reset refactor requested: #15533 (comment)

@cypress
Copy link

cypress bot commented Aug 17, 2022



Test summary

41474 0 1434 0Flakiness 13


Run details

Project cypress
Status Passed
Commit 8b1f855
Started Aug 17, 2022 8:09 PM
Ended Aug 17, 2022 8:25 PM
Duration 15:19 💡
OS Linux Debian - 11.3
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

cypress/e2e/commands/net_stubbing.cy.ts Flakiness
1 network stubbing > intercepting request > can delay with deprecated delayMs param
2 network stubbing > intercepting request > can delay with deprecated delayMs param
3 network stubbing > intercepting request > can delay with deprecated delayMs param
4 network stubbing > intercepting request > can delay with deprecated delayMs param
5 network stubbing > intercepting request > can delay with deprecated delayMs param
This comment includes only the first 5 flaky tests. See all 13 flaky tests in the Cypress Dashboard.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@flotwig flotwig marked this pull request as ready for review August 17, 2022 18:09
ranFirefox: name === 'firefox',
ranElectron: name === 'electron',
ranWebKit: name === 'webkit',
})
Copy link
Contributor

@tbiethman tbiethman Aug 17, 2022

Choose a reason for hiding this comment

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

Props for the simplification here 👍

Copy link
Contributor

@tbiethman tbiethman left a comment

Choose a reason for hiding this comment

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

I've tested this branch on macOS, everything seems as stable as the initial PR. That's a nice assortment of skipped specs; I was hoping there'd be a small number of common differences driving the failures, but it's looking like there's quite a few differences here.

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Nice, tested it out - I still see a weird GraphQL error when I launch the runner with WebKit, I am thinking this is a known bug. I could pitch in and help out with this if needed (as of right now I idea what would be causing it, I didn't look yet).

I also noticed some tests you skipped were passing, eg the shadow DOM one. Maybe it's just flake and I got lucky?

@@ -3928,7 +3929,8 @@ describe('src/cy/commands/actions/click', () => {
})
})

describe('shadow dom', () => {
// TODO(webkit): fix+unskip for experimental webkit
Copy link
Contributor

@lmiller1990 lmiller1990 Aug 18, 2022

Choose a reason for hiding this comment

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

Surprised these tests don't "just work" - I would have thought basic browser APIs would be fairly uniform, since we trigger them with standard client side JS (assuming here, haven't looked at how cy.shadow actually works).

Edit: some actually do just work, or at least, it seems that way on my machine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually this passed for me

image

Copy link
Contributor Author

@flotwig flotwig Aug 18, 2022

Choose a reason for hiding this comment

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

I skipped out the entire click.cy.js because of the large number of failures, I could've been more selective here. This will be unskipped by experimentalWebKitSupport release.

@@ -2609,7 +2617,9 @@ describe('network stubbing', function () {
})

it('intercepts cached responses as expected', {
browser: '!firefox', // TODO: why does firefox behave differently? transparently returns cached response
// TODO: why does firefox behave differently? transparently returns cached response
// TODO(webkit): fix+unskip. currently fails in WK because cache is disabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Is disabling the cache in WebKit something Playwright automation does, or something we do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PW automation.

@lmiller1990
Copy link
Contributor

LGTM, some random questions/nits but no blocker.

@flotwig flotwig merged commit 2724389 into develop Aug 18, 2022
@flotwig flotwig deleted the webkit-prerequests branch August 18, 2022 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants