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] upgrade to playwright 1.28.1 #7696

Merged
merged 12 commits into from
Nov 28, 2022

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Nov 16, 2022

the main issue was that checking the stack trace is unreliable

I also changed the test code to ensure we await the console log

microsoft/playwright#18865 has now been fixed, which was causing across-the-board failures in 1.28.0

the lint failure is unrelated: #7820 (comment)

@changeset-bot
Copy link

changeset-bot bot commented Nov 16, 2022

🦋 Changeset detected

Latest commit: 5b49be8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
create-svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@dominikg
Copy link
Member

triggered a rerun on the mac fail

@benmccann
Copy link
Member Author

Same failure as #7435:

@sveltejs/kit:test: 1 failed
@sveltejs/kit:test: [webkit-dev] › test/client.test.js:610:3 › Load › using window.fetch causes a warning ==========

It seems the webkit version has not been able to capture the console logs

@Conduitry
Copy link
Member

IIRC, the stack trace check in fetcher.js was written that way so as to avoid giving false positives for the warning when a fetch happened to be called when a load was running but was not actually called by that load. Do we really need to lose that heuristic? Have you ever observed that fail when running an app for real, or just in the tests? If at all possible, I'd much rather have imperfect tests than a flawed feature that we can more easily test.

@benmccann
Copy link
Member Author

Thanks. I updated it to keep the stack trace and added a comment explaining what it's doing. It appears to be passing still

Comment on lines +29 to +34
// check if fetch was called via load_node. the lock method only checks if it was called at the
// same time, but not necessarily if it was called from `load`
// we use just the filename as the method name sometimes does not appear on the CI
const heuristic = can_inspect_stack_trace
? stack.includes('src/runtime/client/client.js')
: loading;
Copy link
Member

Choose a reason for hiding this comment

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

is this an async thing? because can_inspect_stack_trace works by checking to see if a newly created stack trace includes the check_stack_trace function

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea. load is contained in the stack trace, but not the others:

@http://localhost:5173/@fs/Users/runner/work/kit/kit/packages/kit/src/runtime/client/fetcher.js:26:49
@http://localhost:5173/src/routes/load/window-fetch/incorrect/+page.js:3:25
load@http://localhost:5173/src/routes/load/window-fetch/incorrect/+page.js:2:28
@http://localhost:5173/@fs/Users/runner/work/kit/kit/packages/kit/src/runtime/client/client.js:653:41

I'm on mobile at the moment so can't easily check whether that's because of async or not, but I'm not sure it'd change the fix? It would be interesting to know though

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can tell, it's not related to async vs sync. load is async and showed up in the stack trace, but the lines before and after are async and the methods didn't show up there.

@Rich-Harris Rich-Harris merged commit 191583c into sveltejs:master Nov 28, 2022
@benmccann benmccann deleted the playwright-1.28 branch November 28, 2022 18:03
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