test: skip reporting CanaryOnly failures for stable version tests #2698
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
pieh
changed the title
📊 Package size report No changes
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
serhalp
previously approved these changes
Oct 24, 2024
| @@ -133,7 +124,7 @@ export class NextDeployInstance extends NextInstance { | |||
|
|
|||
| const deployRes = await execa( | |||
| 'npx', | |||
| ['netlify', 'deploy', '--build', '--json', '--message', deployTitle ?? ''], | |||
There was a problem hiding this comment.
there's a weird bug in ntl deploy --json that makes debugging failures annoying anyway so I like this change :)
(I should write it up but I don't know how to reproduce it 😞)
There was a problem hiding this comment.
I think we are just not getting any output at all on failures with --json flag. At least that's was my experience here and why I wanted to change this stuff in the first place to see actual logs on deploy failures
There was a problem hiding this comment.
yeah, I think that's right... maybe it's by design?
Comment on lines
+121
to
+125
| // skip reporting on tests that even fail to deploy because they rely on experiments not available | ||
| // in currently tested version | ||
| if (testCase.failure?.includes('CanaryOnlyError')) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Doing this here is a little funky because we report test results in 3(?) different places and this is just one of them. So now we'll have a mismatch between the Travis CI summary thing, the actual raw test output, and the e2e report built from this JSON (previously we also had a fourth which was the Slack notification).
Maybe this is fine for now, but I wonder if there's a more direct way to configure the test runner the same way Vercel's CI does? I would imagine it's some slight env var change we need to make or something 🤷🏼.
There was a problem hiding this comment.
https://github.com/vercel/next.js/blob/a8de8730d75625e3bd068787121561cf6ab5eaac/packages/next/src/server/config.ts#L244 setting this env var might prevent build failure and allow actual usage of those features on stable versions (so the deploys wouldn't be failing but features themselves very well might not work correctly yet for us), but I wasn't sure what can of worms I could open when playing with this env var so went with "skipping"
Overall - as we are producing report for users to see and we are generating it for stable version - I think it's fair to skip or ignore tests for features that can't be used on stable versions (without getting into "test mode") - the change here might be not enough overall - but I think direction is correct - skip/ignore them instead of trying to run them?
Those tests would be displayed when we run tests against canary because we wouldn't be hitting CanaryOnly error then
There was a problem hiding this comment.
In any case - this is something we can change, but we need something here and I think this is good enough at least for now until we figure out exactly how we should handle this?
serhalp
approved these changes
Oct 24, 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
This is grab-back of various improvements for running Vercel tests:
run-local-testso that it doesn't require you locally checkout repo to actually be inopennextjs-netlifyand instead just usepwdto locate helper files we copy or useTests
N/A
Relevant links (GitHub issues, etc.) or a picture of cute animal