-
Notifications
You must be signed in to change notification settings - Fork 87
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
chore: multiple fixes to e2e report (#560)
* chore: remove unused deno.json field
When running deno, it warns that this is unused when also specifying `imports`.
* chore: fix type errors in Deno files
We weren't including Deno files in `tsc` typechecking, but we also weren't using any `deno` commands
that typecheck. This fixes obscured errors.
I wanted to introduce typechecking in CI in this commit, but it turned out harder than I expected,
so I've pulled that into a separate PR.
The JSON hack isn't great but really it's the whole "import a json and write new data to it" pattern
that should be simplified to "just read the new data in memory", since there's no actual reason to
write it to disk if you follow the code paths. It was just done that way because it was the shortest
path to add the dynamic fissue annotation from the previous implementation.
* chore(e2e-report): fix handling of skipped suites
Some parts of the code weren't correctly handling the case where an entire suite is skipped, with no
specific skipped tests specified.
1. The `testCount.skipped` before this fix was strangely the number of specifically skipped tests
plus the total number of suites which are either entirely skipped or contain skipped tests (i.e.
it was pretty wrong).
2. The computed `skipped` count within each test suite was also wrong but in a different way. It was
not counting at all skipped tests where the entire suite is skipped.
I've also commented some existing correct logic that is very unintuitive.
* chore(e2e): split open issues and skipped tests
It was really confusing, both as a person viewing the page, and as a developer working with the
code. This code and this component were muddling three different data types: failed tests, skipped
tests, and skipped test suites. It was really error prone and hard to follow and modify.
I've split this into separate components and sections on the page for failed tests ("open issues")
and skipped tests, and I've passed skipped tests and skipped suites separately into the latter
component.
I've also refactored some code with side effects to be easier to work with.
* refactor(e2e): remove stale todo
* chore(e2e): update e2e test-results.json fixture
This is the latest run on "latest" against main, from last night.
* chore: remove unused deno scripts
* chore(e2e): add test-results.json fixture to second path
We expect this in two different locations depending on the code path... Having a sample file here
allows for TypeScript to infer the type correctly.
* chore(e2e): add back some skipped tests we removed
The tests were named on canary, but we still need the old test names for `latest`.
* chore(e2e): fix bad skipped test name, skip 2 more
* chore(e2e): add missing tests to e2e-skip-retry.json
* chore(e2e): add one more skipped test
* chore(e2e): fix appending of skipped suites
It was appending entirely skipped suites AND partially skipped suites.
* chore(e2e): update test-results.json fixtures again
* chore(e2e): add sample issues.json for local dev- Loading branch information
Showing
19 changed files
with
13,715 additions
and
5,478 deletions.
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,12 @@ | ||
| { | ||
| "lint": { | ||
| "files": { | ||
| "include": ["edge-runtime/middleware.ts"] | ||
| "include": [ | ||
| "edge-runtime/middleware.ts" | ||
| ] | ||
| } | ||
| }, | ||
| "imports": { | ||
| "@netlify/edge-functions": "https://edge.netlify.com/v1/index.ts" | ||
| }, | ||
| "importMap": "./edge-runtime/vendor/import_map.json" | ||
| } | ||
| } |
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| [ | ||
| { | ||
| "body": "I'm not quite sure exactly what the scope of this is, but the failing tests here have a fixture site that enables `skipMiddlewareUrlNormalize` and a user middleware that redirects any other casing of `/en/*` to `/en/*` (and so on), but when `/EN` is fetched it redirects to `/en/en` instead of `/en`:\r\n\r\n```\r\n ● skip-trailing-slash-redirect › should be able to redirect locale casing $1\r\n\r\n expect(received).toBe(expected) // Object.is equality\r\n\r\n Expected: \"/en\"\r\n Received: \"/en/en\"\r\n\r\n 155 | const res = await next.fetch(`/${locale}`, { redirect: 'manual' })\r\n 156 | expect(res.status).toBe(307)\r\n > 157 | expect(new URL(res.headers.get('location'), 'http://n').pathname).toBe(\r\n | ^\r\n 158 | `/${locale.toLowerCase()}`\r\n 159 | )\r\n 160 | }\r\n\r\n at toBe (e2e/skip-trailing-slash-redirect/index.test.ts:157:73)\r\n\r\n ● skip-trailing-slash-redirect › should be able to redirect locale casing $1\r\n\r\n expect(received).toBe(expected) // Object.is equality\r\n\r\n Expected: \"/ja-jp\"\r\n Received: \"/ja-jp/ja-jp\"\r\n\r\n 155 | const res = await next.fetch(`/${locale}`, { redirect: 'manual' })\r\n 156 | expect(res.status).toBe(307)\r\n > 157 | expect(new URL(res.headers.get('location'), 'http://n').pathname).toBe(\r\n | ^\r\n 158 | `/${locale.toLowerCase()}`\r\n 159 | )\r\n 160 | }\r\n\r\n at toBe (e2e/skip-trailing-slash-redirect/index.test.ts:157:73)\r\n```\r\n\r\nIt looks like we [previously claimed to have fixed this in FRA-332](https://github.com/netlify/next-runtime-minimal/pull/287), but it must have regressed at some point.\r\n\r\ntest: test/e2e/skip-trailing-slash-redirect/index.test.ts\r\nreason: does not correctly handle user middleware that redirects to path with canonical locale casing when app enables `skipMiddlewareUrlNormalize` and path contains locale slug with non-canonical casing", | ||
| "number": 564 | ||
| }, | ||
| { | ||
| "body": "A pages router site with both basepath and i18n enabled does not match middleware targeted at the root unless a locale is in the URL. It should match when using the default locale\r\n\r\nDemo: \r\n- https://6634b69452829cd391cab206--next-e2e-tests.netlify.app/root/ does not have `x-from-middleware` header\r\n- https://6634b69452829cd391cab206--next-e2e-tests.netlify.app/root/en/ does have `x-from-middleware` header\r\n\r\ntest: test/e2e/middleware-matcher/index.test.ts\r\nreason: Middleware does not match when using basePath and default locale", | ||
| "number": 454 | ||
| }, | ||
| { | ||
| "body": "There are a few scenarios where our the custom matchers from middleware are not correctly being transformed when using i18n and default locales, as they're either matching too broadly or too narrowly. See the two linked GH issues for details:\r\n\r\n[https://github.com/netlify/next-runtime-minimal/issues/453](https://github.com/netlify/next-runtime-minimal/issues/453)<br>[https://github.com/netlify/next-runtime-minimal/issues/454](https://github.com/netlify/next-runtime-minimal/issues/454)\r\n\r\ntest: Middleware custom matchers i18n should not match\r\nreason: Middleware matching is too broad when using i18n", | ||
| "number": 453 | ||
| }, | ||
| { | ||
| "body": "On sites that use pages router and have middleware, loading a page using next/link will attempt to load a JSON file, which will return a 404. If there is no middleware then it works fine. This applies even if the middleware does nothing.\r\n\r\ntest: test/e2e/middleware-base-path/test/index.test.ts\r\nreason: Pages router data requests returning 404 when middleware is used\r\ntest case: \"Middleware base tests router.query must exist when Link clicked page routing\"", | ||
| "number": 450 | ||
| }, | ||
| { | ||
| "body": "When appending a `set-cookie` header, the server returns two copies of the header. e.g. \r\n\r\n```ts\r\nexport async function middleware(request, ev) {\r\n const next = NextResponse.next()\r\n next.headers.append('set-cookie', 'bar=chocochip')\r\n return next\r\n}\r\n```\r\n\r\nLeads to: \r\n\r\n\r\n\r\ntest: test/e2e/middleware-responses/test/index.test.ts\r\nreason: Appending set-cookie header in middleware leads to duplicate header\r\n", | ||
| "number": 447 | ||
| }, | ||
| { | ||
| "body": "The test [\"app-dir action handling fetch actions should store revalidation data in the prefetch cache\"](https://github.com/vercel/next.js/blob/6475431a4cbbf2b71c38158e0e722183779faf4f/test/e2e/app-dir/actions/app-action.test.ts#L980) fails when running tests, but seems to work fine when testing manually. It may be a first-run issue, which needs investigation.\r\n\r\ntest: test/e2e/app-dir/actions/app-action.test.ts\r\nreason: Fetch action prefetch cache test is flakey", | ||
| "number": 444 | ||
| }, | ||
| { | ||
| "body": "If a Next.js page returns a 500 error and the browser sent accept-encoding gzip, it seems the returned data _is_ encoded, but no encoding header is set, so the browser cannot decode it.\r\n\r\n`curl --request GET --url https://66056f2be8186e00aaea53d3--next-e2e-tests.netlify.app/enoent --header 'Accept-Encoding: gzip'`\r\n\r\ntest case: https://github.com/vercel/next.js/blob/canary/test/e2e/getserversideprops/test/index.test.ts#L367\r\ntest: test/e2e/getserversideprops/test/index.test.ts\r\nreason: Server error pages return encoded data without content-encoding header if accept-encoding is gzip", | ||
| "number": 387 | ||
| }, | ||
| { | ||
| "body": "When redirecting a data request, middleware returns a response with `x-nextjs-redirect`, rather than a `location` header. We handle this correctly. However Next.js expects us to directly return and empty response with a 302 response code (without the location header), whereas we're currently passing the request on to the origin and returning the body with 404 code. I'm unsure if it's legal to return a 302 with no location, but it's what next start does, and the router expects.\r\n\r\ntest case: https://github.com/vercel/next.js/blob/canary/test/e2e/middleware-redirects/test/index.test.ts#L100\r\ntest: test/e2e/middleware-redirects/test/index.test.ts\r\nreason: Pages router middleware should return 302 status for redirected data requests", | ||
| "number": 386 | ||
| }, | ||
| { | ||
| "body": "In sites with trailing slashes enabled, requests should not add a trailing slash when they're for non-data static files.\r\n\r\ntest case: https://github.com/vercel/next.js/blob/canary/test/e2e/middleware-trailing-slash/test/index.test.ts#L436-L437\r\ntest: test/e2e/middleware-trailing-slash/test/index.test.ts\r\nreason: Middleware should not add trailing slashes to non-data requests in static dir", | ||
| "number": 385 | ||
| }, | ||
| { | ||
| "body": "Rewrites in middleware on i18n sites add the locale to the target automatically. However it is supposed to skip this if the target is a static file. This does not currently work, as the middleware doesn't know if a static file exists. This is documented as a known issue.\r\n\r\ntest: test/e2e/i18n-ignore-rewrite-source-locale/rewrites-with-basepath.test.ts, test/e2e/i18n-ignore-rewrite-source-locale/rewrites.test.ts\r\nreason: Middleware on sites with i18n cannot rewrite to static files", | ||
| "number": 383 | ||
| }, | ||
| { | ||
| "body": "It doesn't seem to be documented, but the e2e test and fixture here expects a CSP to be automatically applied to scripts in the head. The docs show manually setting it, but [the fixture](https://github.com/vercel/next.js/blob/canary/test/e2e/app-dir/app/middleware.js#L30) seems to only set it in the response. Nevertheless, running `next start` does seem to set it automatically, but when deployed it doesn't. I think this is low priority because it seems to be undocumented behaviour.\r\n\r\ntest case: https://github.com/vercel/next.js/blob/canary/test/e2e/app-dir/app/index.test.ts#L1711\r\ntest: test/e2e/app-dir/app/index.test.ts\r\nreason: Nonce not automatically set in script tags when using CSP", | ||
| "number": 381 | ||
| } | ||
| ] |
Oops, something went wrong.