-
Notifications
You must be signed in to change notification settings - Fork 91
test: deflake and another next 16 compat update for fixtures #3181
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
📊 Package size report 0.01%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
| expect(response2?.status()).toBe(200) | ||
| expect(response2?.headers()['cache-status']).toMatch( | ||
| /("Netlify Edge"; hit; fwd=stale|"Netlify Durable"; hit; ttl=-[0-9]+)/m, | ||
| /("Netlify Edge"; fwd=stale|"Netlify Durable"; hit; ttl=-[0-9]+)/m, |
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 Netlify Edge one was wrong for quite some time so this in practice was relying on hitting durable cache case
| } catch (error) { | ||
| let shouldRetry = false | ||
| // not ideal, but there is no error code for that | ||
| if (error.message === 'fetch failed' && attempt < NUM_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.
this is happening very randomly and does not seem related to any specific fetch target:
- it might be happening in fetch that CLI is doing to fetch extensions and result in confusing error about site not being linked (even tho we specifically pass site_id to cli command in our e2e tests)
- it might be happening in data fetches in fixtures (both when preparing fixtures for integration tests and when building and deploying for e2e tests)
it seems more related to runners and hitting potential limits there
PS. I do hate it, but have no better idea as this would have to be replicated in a lot of places, some of which we don't control
| @@ -1,4 +1,4 @@ | |||
| import { unstable_cacheLife as cacheLife, unstable_cacheTag as cacheTag } from 'next/cache' | |||
| import { cacheLife, cacheTag } from '../../../../../next-cache' | |||
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.
unstable_cacheLife no longer exist in next@canary - it's just cacheLife now
But because we still will be running tests for next@15 I did add helper module for compatibility that will use either export with unstable_ prefix or stable one, depending on which is available in tested next.js version
| env: { | ||
| NX_ISOLATE_PLUGINS: 'false', | ||
| }, | ||
| }), | ||
| nxIntegratedDistDir: () => | ||
| createE2EFixture('nx-integrated', { | ||
| packagePath: 'apps/custom-dist-dir', | ||
| buildCommand: 'nx run custom-dist-dir:build', | ||
| publishDirectory: 'dist/apps/custom-dist-dir/dist', | ||
| env: { | ||
| NX_ISOLATE_PLUGINS: 'false', | ||
| }, |
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.
nx on next canary starting failing with errors mentioned in nrwl/nx#27040 (but it's not exactly the same, because issue mention postinstall, while for us errors were happening at next build time, so despite that issue being closed with a fix, I think we are hitting similar problem in different place). I tried NX_ISOLATE_PLUGINS which was suggested as workaround there and that seemed to work here as well
b430591 to
0e951a2
Compare
|
I'll close this as some adjustments were merged separately and some are included in #3180 |
Description
Documentation
Tests
You can test this change yourself like so:
Relevant links (GitHub issues, etc.) or a picture of cute animal