-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
chore: test matrix dimension for vite #15208
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
Conversation
|
|
Looks like this is failing the CI: But happy to merge it once it's passing |
|
Something seems to still be freaking out, looks like a broken lockfile replacement maybe? |
|
the baseline test passed locally for me using node18, not sure what difference CI makes here. |
…ace consistency, add test matrix dimension and tests for vite5 and vite8
…heck in types test
…nsure tests work with retry and conters are independent
2557112 to
710e60a
Compare
…kip the test anyways and it would fail
| - node-version: 18 | ||
| os: ubuntu-latest | ||
| e2e-browser: 'chromium' | ||
| vite: 'baseline' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
introduces a new matrix dimension called vite with 3 values
baseline: the lowest version of vite we support (peer range of sveltekit)
current: the version of vite currently in the lockfile
beta: the upcoming beta version of vite
| - name: setup overrides for matrix.vite | ||
| if: matrix.vite != 'current' | ||
| run: | # copies catalogs.vite-xxx to overrides in pnpm-workspace.yaml so the subsequent `pnpm install` enforces them | ||
| yq -i '.overrides =.catalogs."vite-${{matrix.vite}}"' pnpm-workspace.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yq is available on github hosted runners (except windows) and allows in place editing of yaml
this code copies a block of catalogs.vite-baseline into overrides so tha the install that we run afterwards enforces these versions. see pnpm-workspace.yaml for details
| matrix: | ||
| include: | ||
| - node-version: 18 | ||
| - node-version: 24 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node 18 caused build fails with newer versions of vite-plugin-svelte@6 that uses node:util styleText.
these tests are for browser behavior so i think its ok to update to 24 which should make them a bit more efficient (24 is faster than 18)
same goes for vite6
| - name: setup overrides for matrix.node | ||
| if: matrix.node-version == 18 | ||
| run: | # copies catalogs.node-xx to overrides in pnpm-workspace.yaml so the subsequent `pnpm install` enforces them | ||
| yq -i '.overrides =.catalogs."node-18"' pnpm-workspace.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to vite-baseline we have to pick versions that support node18 here otherwise it can break.
| @@ -1,5 +1,5 @@ | |||
| <script> | |||
| import { get_event } from './data.remote.js'; | |||
| import { get_event } from './data.remote.ts'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concession change for vite5, it doesn't work with data.remote.js on node 18, allowing .ts import and changing the imported suffix works.
| const cwd = path.join(directories, dir); | ||
|
|
||
| if (!fs.existsSync('svelte.config.js')) { | ||
| if (!fs.existsSync(path.join(cwd,'svelte.config.js'))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without this, sync-all does nothing which caused one test with vite8 to fail because rolldown does not like missing .svelte-kit/tsconfig.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the missing tsconfig causing an error has been fixed in latest vite8 beta, but this fix to sync-all should still be done
|
one thing i havn't fully wrapped my head around yet is if these overrides could lead to dependency cache poisoning. that could happen if pnpm action pruned unused vite from the cached pnpm store and a test with these overrides finished first and gets to write it. |
|
Seems like one of the CI checks is failing consistently. Something about $app/read not working. |
|
it's been passing for me locally so i kind of have a hard time reproducing/analyzing it. might have to download the playwright trace and go from there. If anyone can replicate by using node18 an the overrides from the baseline catalog please help :) |
packages/kit/test/utils.js
Outdated
| port: process.env.DEV ? 5173 : 4173 | ||
| }, | ||
| retries: process.env.CI ? 2 : 0, | ||
| retries: 2, // some tests are flaky especially on overloaded systems, so allow retry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I preferred it failing locally as it was easier to see what was going wrong and gave more incentive to fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure that incentive is working then because they failed on me without any changes from main more often than not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benmccann i can change it to
/**
* @param {string} name
* @param {number} default_value
* @return {number}
*/
function number_from_env(name,default_value) {
return ( process.env[name] != null) ? Number(process.env[name]) : default_value
}
/* ...*/
retries: process.env.CI ? 2 : number_from_env('SVELTEKIT_PLAYWRIGHT_RETRIES',0),
so you could run it with SVELTEKIT_PLAYWRIGHT_RETRIES=2 pnpm test:kit locally but default would still be no retries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for the workers change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that works for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added KIT_E2E_WORKERS and KIT_E2E_RETRIES (we already had KIT_E2E_BROWSER) in all configs and a shared util to read number from env
the util lives somewhat awkwardly in the root, but i didn't want to introduce a workspace package. If anyone has ideas how to better share it or if it should be inlined into the 4 files thats fine with me too
Co-authored-by: Ben McCann <[email protected]>
we are using the cache feature of These tests are also slower than others that use the cache with the same key so they are very unlikely to succeed in trying to store it. If we still feel this is too risky, we'd have to disable caching on setup-node if vite dimension is not current |
teemingc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
this allows us to test with different vite majors
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Edits