Skip to content

test(e2e): fix failures due to latest and canary releases #2587

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 17, 2024
Merged

test(e2e): fix failures due to latest and canary releases #2587

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a318ab
test(e2e): fix failures due to latest and canary releases
serhalp Sep 12, 2024
d8f0bba
fix: fix private directive logic in tests
serhalp Sep 17, 2024
b8110b1
test: update one more no-cache test
serhalp Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions tests/e2e/cli-before-regional-blobs-support.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { expect } from '@playwright/test'
import { test } from '../utils/playwright-helpers.js'
import { nextVersionSatisfies } from '../utils/next-version-helpers.mjs'

test('should serve 404 page when requesting non existing page (no matching route) if site is deployed with CLI not supporting regional blobs', async ({
page,
Expand All @@ -18,8 +19,17 @@ test('should serve 404 page when requesting non existing page (no matching route

expect(await page.textContent('h1')).toBe('404')

// https://github.com/vercel/next.js/pull/69802 made changes to returned cache-control header,
// after that (14.2.10 and canary.147) 404 pages would have `private` directive, before that it
// would not
const shouldHavePrivateDirective = nextVersionSatisfies(
'>=14.2.10 <15.0.0 || >=15.0.0-canary.147',
)
expect(headers['netlify-cdn-cache-control']).toBe(
'no-cache, no-store, max-age=0, must-revalidate, durable',
(shouldHavePrivateDirective ? 'private, ' : '') +
'private, no-cache, no-store, max-age=0, must-revalidate, durable',
)
expect(headers['cache-control']).toBe(
(shouldHavePrivateDirective ? 'private,' : '') + 'no-cache,no-store,max-age=0,must-revalidate',
)
expect(headers['cache-control']).toBe('no-cache,no-store,max-age=0,must-revalidate')
})
28 changes: 24 additions & 4 deletions tests/e2e/page-router.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { expect } from '@playwright/test'
import { test } from '../utils/playwright-helpers.js'
import { nextVersionSatisfies } from '../utils/next-version-helpers.mjs'

export function waitFor(millis: number) {
return new Promise((resolve) => setTimeout(resolve, millis))
Expand Down Expand Up @@ -340,10 +341,20 @@ test.describe('Simple Page Router (no basePath, no i18n)', () => {

expect(await page.textContent('h1')).toBe('404')

// https://github.com/vercel/next.js/pull/69802 made changes to returned cache-control header,
// after that (14.2.10 and canary.147) 404 pages would have `private` directive, before that
// it would not
const shouldHavePrivateDirective = nextVersionSatisfies(
'>=14.2.10 <15.0.0 || >=15.0.0-canary.147',
)
expect(headers['netlify-cdn-cache-control']).toBe(
'no-cache, no-store, max-age=0, must-revalidate, durable',
(shouldHavePrivateDirective ? 'private, ' : '') +
'no-cache, no-store, max-age=0, must-revalidate, durable',
)
expect(headers['cache-control']).toBe(
(shouldHavePrivateDirective ? 'private,' : '') +
'no-cache,no-store,max-age=0,must-revalidate',
)
expect(headers['cache-control']).toBe('no-cache,no-store,max-age=0,must-revalidate')
})

test('should serve 404 page when requesting non existing page (marked with notFound: true in getStaticProps)', async ({
Expand Down Expand Up @@ -1039,10 +1050,19 @@ test.describe('Page Router with basePath and i18n', () => {

expect(await page.textContent('h1')).toBe('404')

// https://github.com/vercel/next.js/pull/69802 made changes to returned cache-control header,
// after that 404 pages would have `private` directive, before that it would not
const shouldHavePrivateDirective = nextVersionSatisfies(
'>=14.2.10 <15.0.0 || >=15.0.0-canary.147',
)
expect(headers['netlify-cdn-cache-control']).toBe(
'no-cache, no-store, max-age=0, must-revalidate, durable',
(shouldHavePrivateDirective ? 'private, ' : '') +
'no-cache, no-store, max-age=0, must-revalidate, durable',
)
expect(headers['cache-control']).toBe(
(shouldHavePrivateDirective ? 'private,' : '') +
'no-cache,no-store,max-age=0,must-revalidate',
)
expect(headers['cache-control']).toBe('no-cache,no-store,max-age=0,must-revalidate')
})

test('requesting a non existing page route that needs to be fetched from the blob store like 404.html (notFound: true)', async ({
Expand Down
8 changes: 6 additions & 2 deletions tests/e2e/simple-app.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,12 @@ test('requesting a non existing page route that needs to be fetched from the blo
expect(await page.textContent('h1')).toBe('404 Not Found')

// https://github.com/vercel/next.js/pull/66674 made changes to returned cache-control header,
// before that 404 page would have `private` directive, after that it would not
const shouldHavePrivateDirective = !nextVersionSatisfies('>=14.2.4 <15.0.0 || >=15.0.0-canary.24')
// before that 404 page would have `private` directive, after that (14.2.4 and canary.24) it
// would not ... and then https://github.com/vercel/next.js/pull/69802 changed it back again
// (14.2.10 and canary.147)
const shouldHavePrivateDirective = nextVersionSatisfies(
'<14.2.4 || >=14.2.10 < 15 || <15.0.0-canary.24 || >= 15.0.0-canary.147',
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition is different than one used in other places - because this one has <15.0.0-canary.24 without lower limit, it will actually match 14.2.5 for example (tho we won't be really hitting those edge cases with the version we test setup as all currently tested version would have private directive (13.5.1, latest which is now 14.2.11 and canary which is now 15.0.0-canary.157)

This would work (and then probably same condition should be used across the board? maybe we centralize it and add shouldHavePrivateDirective to same helper module that already has nextVersionSatisfies helper?)

Suggested change
'<14.2.4 || >=14.2.10 < 15 || <15.0.0-canary.24 || >= 15.0.0-canary.147',
'<14.2.4 || >=14.2.10 <15.0.0-canary.24 || >= 15.0.0-canary.147',

Or we use the condition used elsewhere with understanding that those earlier canary versions (>=15.0.0-canary.0 <15.0.0-canary.24) generally should have private directive but we skip those because we don't really test those anymore and keeping condition cleaner and easier to reason with

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. I've read those semver docs a hundred times and I still keep finding things I've misinterpreted. I thought prerelease range specifiers could never match stable releases (and prereleases with a different prefix).

That clearly wasn't right:

$ npm i -g semver
$ semver -r '<15.0.0-canary.24' '14.2.5' '15.2.5' '15.0.0-canary.20' '15.0.0-canary.30'
14.2.5
15.0.0-canary.20

Copy link
Member Author

@serhalp serhalp Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Fascinating:

$ semver -r '<15.0.0' 15.0.0-canary.150
15.0.0-canary.150

Maybe this was obvious to you, but I thought this behaved kind of like JS's comparison operators with values like NaN:

> 3 < NaN
false
> 3 > NaN
false

Copy link
Contributor

@pieh pieh Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's a bit more nuanced (or maybe some other word that could fit here better 🙃 ):

semver -r '<15.0.0-canary.24' '14.2.5' '14.2.5-canary.5' '15.2.5'  '15.0.0-canary.20' '15.0.0-canary.30'
14.2.5
15.0.0-canary.20

The 14.2.5-canary.5 wouldn't match but 14.2.5 would ... unless you would use includePrerelease:

semver --include-prerelease -r '<15.0.0-canary.24' '14.2.5' '14.2.5-canary.5' '15.2.5'  '15.0.0-canary.20' '15.0.0-canary.30'
14.2.5-canary.5
14.2.5
15.0.0-canary.20

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe we centralize it and add shouldHavePrivateDirective to same helper module that already has nextVersionSatisfies helper?

I had thought about that, but strangely we'd need two different versions of it (some assertions are affected by the change a few months ago, some are affected by both changes) and I couldn't come up with a reasonable name for these functions I decided this was a case of "a poor abstraction is worse than no abstraction".

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a bit more nuanced [...]

Somehow I knew about that nuance but not the more fundamental one above!

)

expect(headers['netlify-cdn-cache-control']).toBe(
(shouldHavePrivateDirective ? 'private, ' : '') +
Expand Down
Loading