test(e2e): fix failures due to latest and canary releases #2587
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
serhalp
added
the
test all versions
📊 Package size report No changes
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
[v14.2.10](https://github.com/vercel/next.js/releases/tag/v14.2.10) and [v15.0.0-canary.147](https://github.com/vercel/next.js/releases/tag/v15.0.0-canary.147) included vercel/next.js#69802 which added `private` back into `no-cache,no-store` `cache-control` headers in some cases.
serhalp
force-pushed
the
test/fix-e2e-for-latest-nextjs
branch
from
db0ec61
to
5a318ab
Compare
❌ Deploy Preview for next-runtime-turbofan failed. Why did it fail? →
|
pieh
reviewed
Sep 17, 2024
pieh
reviewed
Sep 17, 2024
tests/e2e/simple-app.test.ts
Outdated
| // 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', |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
It's a bit more nuanced [...]
Somehow I knew about that nuance but not the more fundamental one above!
pieh
approved these changes
Sep 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
v14.2.10 and v15.0.0-canary.147 included vercel/next.js#69802 which added
privateback intono-cache,no-storecache-controlheaders in some cases.There are still failures after this fix but they are caused by two (three?) additional unrelated issues.
Documentation
N/A
Tests
N/A
Relevant links (GitHub issues, etc.) or a picture of cute animal
N/A