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: separate out cross-browser tests #8585

Merged
merged 12 commits into from
Jan 19, 2023
Merged

chore: separate out cross-browser tests #8585

merged 12 commits into from
Jan 19, 2023

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Jan 18, 2023

I thought I'd share this for discussion as it may be a bit easier to evaluate the idea while being able to look at a potential breakdown of which tests would be run cross-browser. I've got roughly half the tests running cross-browser (i.e. on WebKit). We could probably fine tune a bit which ones live where and I'm totally open to moving tests from one file to another.

This should also cut down our quota usage significantly since macOS is charged at a 10x rate

Closes #8578

@changeset-bot
Copy link

changeset-bot bot commented Jan 18, 2023

⚠️ No Changeset found

Latest commit: 97ece7d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@Rich-Harris
Copy link
Member

I think we can go further — there's no need to run unit tests (in kit or any other package), or anything that isn't a cross-browser test (including everything in server.test.js, plus prerendering tests etc), more than once. So maybe we run the full suite in 16, ubuntu-latest, chromium and in all the others we only run cross-browser tests?

@Rich-Harris
Copy link
Member

gah looks like the symlinks need to be fixed. will do in the morning if no-one beats me to it

@@ -9,7 +9,10 @@
"check": "svelte-kit sync && tsc && svelte-check",
"test": "node test/setup.js && pnpm test:dev && pnpm test:build",
"test:dev": "rimraf test/errors.json && cross-env DEV=true playwright test",
"test:build": "rimraf test/errors.json && playwright test"
"test:build": "rimraf test/errors.json && playwright test",
Copy link
Member Author

Choose a reason for hiding this comment

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

I notice that writes/package.json also does rimraf test/errors.json despite it only being created in the basics test, so perhaps we could clean remove it from that file

Copy link
Member

Choose a reason for hiding this comment

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

done. would love to find a way to not need that errors.json thing, it's super awkward

Copy link
Member

Choose a reason for hiding this comment

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

i wonder if we could put the error on event.locals then return it with the response via a x-error header or something. might try that after we merge this change

turbo.json Outdated Show resolved Hide resolved
turbo.json Show resolved Hide resolved
Copy link
Member Author

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

lgtm. thanks for the cleanup

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.

Reduce the use of Webkit in browser tests
2 participants