Skip to content
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

test: add feature of overriding files #74716

Merged
merged 4 commits into from
Jan 10, 2025

Conversation

huozhi
Copy link
Member

@huozhi huozhi commented Jan 9, 2025

What

  • For testing, we add a overrideFiles to nextTestSetup to override the local files to easily create vary test cases. This was a small improvement for testing util I planned before. (Closes NDX-323). So you can have 2 test files for one test fixture, and override the file with overridingFiles. Same usage as files.
  • Update the bad test condition where it was accidentally get skipped. Also fix the bad snapshot

###. Example

For instance, if you want to use the same app fixture but different next config, just override the config. You don't have to stop the nextjs instance and modify then restart. Time saver.

const { next } = nextTestSetup({
      files: __dirname,
      overrideFiles: {
        'next.config.js': `
        /**
         * @type {import('next').NextConfig}
         */
        const nextConfig = {
          experimental: {
            reactOwnerStack: false,
          },
        }
        module.exports = nextConfig
        `,
      },
     // ...

@ijjk ijjk added created-by: Next.js team PRs by the Next.js team. tests labels Jan 9, 2025
Copy link
Member Author

huozhi commented Jan 9, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@huozhi huozhi marked this pull request as ready for review January 9, 2025 22:52
@ijjk
Copy link
Member

ijjk commented Jan 9, 2025

Tests Passed

@huozhi huozhi requested a review from ijjk January 9, 2025 22:52
@ijjk
Copy link
Member

ijjk commented Jan 9, 2025

Stats from current PR

Default Build (Increase detected ⚠️)
General
vercel/next.js canary vercel/next.js huozhi/01-09-test_support_overrides Change
buildDuration 31.3s 26.2s N/A
buildDurationCached 25s 22.3s N/A
nodeModulesSize 416 MB 416 MB
nextStartRea..uration (ms) 777ms 789ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js huozhi/01-09-test_support_overrides Change
5306-HASH.js gzip 53.3 kB 53.3 kB N/A
8276.HASH.js gzip 169 B 168 B N/A
8377-HASH.js gzip 5.44 kB 5.44 kB N/A
bccd1874-HASH.js gzip 53 kB 53 kB
framework-HASH.js gzip 57.5 kB 57.5 kB N/A
main-app-HASH.js gzip 240 B 242 B N/A
main-HASH.js gzip 34.1 kB 34.1 kB N/A
webpack-HASH.js gzip 1.71 kB 1.71 kB N/A
Overall change 53 kB 53 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js huozhi/01-09-test_support_overrides Change
polyfills-HASH.js gzip 39.4 kB 39.4 kB
Overall change 39.4 kB 39.4 kB
Client Pages
vercel/next.js canary vercel/next.js huozhi/01-09-test_support_overrides Change
_app-HASH.js gzip 193 B 193 B
_error-HASH.js gzip 193 B 193 B
amp-HASH.js gzip 512 B 510 B N/A
css-HASH.js gzip 343 B 342 B N/A
dynamic-HASH.js gzip 1.84 kB 1.84 kB
edge-ssr-HASH.js gzip 265 B 265 B
head-HASH.js gzip 363 B 362 B N/A
hooks-HASH.js gzip 393 B 392 B N/A
image-HASH.js gzip 4.57 kB 4.57 kB N/A
index-HASH.js gzip 268 B 268 B
link-HASH.js gzip 2.35 kB 2.34 kB N/A
routerDirect..HASH.js gzip 328 B 328 B
script-HASH.js gzip 397 B 397 B
withRouter-HASH.js gzip 323 B 326 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 3.59 kB 3.59 kB
Client Build Manifests
vercel/next.js canary vercel/next.js huozhi/01-09-test_support_overrides Change
_buildManifest.js gzip 749 B 747 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js huozhi/01-09-test_support_overrides Change
index.html gzip 524 B 524 B
link.html gzip 538 B 538 B
withRouter.html gzip 520 B 520 B
Overall change 1.58 kB 1.58 kB
Edge SSR bundle Size
vercel/next.js canary vercel/next.js huozhi/01-09-test_support_overrides Change
edge-ssr.js gzip 128 kB 128 kB N/A
page.js gzip 207 kB 207 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js huozhi/01-09-test_support_overrides Change
middleware-b..fest.js gzip 671 B 669 B N/A
middleware-r..fest.js gzip 155 B 156 B N/A
middleware.js gzip 31.2 kB 31.2 kB N/A
edge-runtime..pack.js gzip 844 B 844 B
Overall change 844 B 844 B
Next Runtimes
vercel/next.js canary vercel/next.js huozhi/01-09-test_support_overrides Change
274-experime...dev.js gzip 322 B 322 B
274.runtime.dev.js gzip 314 B 314 B
app-page-exp...dev.js gzip 367 kB 367 kB
app-page-exp..prod.js gzip 129 kB 129 kB
app-page-tur..prod.js gzip 142 kB 142 kB
app-page-tur..prod.js gzip 138 kB 138 kB
app-page.run...dev.js gzip 355 kB 355 kB
app-page.run..prod.js gzip 126 kB 126 kB
app-route-ex...dev.js gzip 37.6 kB 37.6 kB
app-route-ex..prod.js gzip 25.6 kB 25.6 kB
app-route-tu..prod.js gzip 25.6 kB 25.6 kB
app-route-tu..prod.js gzip 25.4 kB 25.4 kB
app-route.ru...dev.js gzip 39.2 kB 39.2 kB
app-route.ru..prod.js gzip 25.4 kB 25.4 kB
pages-api-tu..prod.js gzip 9.69 kB 9.69 kB
pages-api.ru...dev.js gzip 11.6 kB 11.6 kB
pages-api.ru..prod.js gzip 9.68 kB 9.68 kB
pages-turbo...prod.js gzip 21.7 kB 21.7 kB
pages.runtim...dev.js gzip 27.5 kB 27.5 kB
pages.runtim..prod.js gzip 21.7 kB 21.7 kB
server.runti..prod.js gzip 916 kB 916 kB
Overall change 2.46 MB 2.46 MB
build cache Overall increase ⚠️
vercel/next.js canary vercel/next.js huozhi/01-09-test_support_overrides Change
0.pack gzip 2.09 MB 2.09 MB ⚠️ +2.79 kB
index.pack gzip 75.3 kB 74.9 kB N/A
Overall change 2.09 MB 2.09 MB ⚠️ +2.79 kB
Diff details
Diff for main-HASH.js

Diff too large to display

Commit: 8897b52

@huozhi huozhi requested review from gaojude and ztanner January 9, 2025 23:22
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Makes sense that we no longer cleanup like you did with your previous setup. When I look at the app after the test, I'd expect the same initial state as the test had. This wasn't the case before since you cleaned it up in afterAll

Though the wasteful next.stop could've been avoided with skipStart: true and a manual next.start(). But overrideFiles makes the intent clearer.

Just need to be careful not to overuse. It's probably only valid for config flags and I've seen people use env variables for flags instead e.g.

env: { TEST_DATA_SERVER: `http://localhost:${port}/` },
or
env: {
ANOTHER_MIDDLEWARE_TEST: 'asdf2',
STRING_ENV_VAR: 'asdf3',
MIDDLEWARE_TEST: 'asdf',
},
. So this is adding another pattern which should be carefully considered.

@unstubbable
Copy link
Contributor

unstubbable commented Jan 10, 2025

Just need to be careful not to overuse. It's probably only valid for config flags and I've seen people use env variables for flags instead. [...] So this is adding another pattern which should be carefully considered.

+1 on this. By overwriting files in tests it makes it unnecessarily complicated to run the test app with pnpm next dev using the same config or fixtures as used in the test.

@huozhi
Copy link
Member Author

huozhi commented Jan 10, 2025

Just need to be careful not to overuse. It's probably only valid for config flags and I've seen people use env variables for flags instead e.g.

I do feel we need this not only limited to config but also page/layout files, sometimes you might need to chaneg a runtime config to edge and re-run it. The tests results usually won't change but the varied configuration is in page/layout files.

it makes it unnecessarily complicated to run the test app with pnpm next dev using the same config or fixtures as used in the test.
I don't think the goal is to duplicate all the test fixtures for all cases, like the edge case I mentioned above or any other non-next-config configuration that need to be set in the page. Or perhapse just some render condition in one file, which is unnecessary to duplicate the whole test fixture.

The control of usage is in our own hand, we can definitely raise concerns when seeing absue in the tests and request to change or add another test fixture. This is just another small convenience like the existing ones such as allowing you to use string for simple test fixture file in the setup.

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

I think being able to override a specific file and re-use a fixture makes sense

@huozhi huozhi merged commit 08c6275 into canary Jan 10, 2025
132 checks passed
@huozhi huozhi deleted the huozhi/01-09-test_support_overrides branch January 10, 2025 18:39
huozhi added a commit that referenced this pull request Jan 17, 2025
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants