Skip to content

Commit 2a29c29

Browse files
RobinMalfaitRobinMalfait
authored
Improve integration tests (stability + performance) (#15125)
This PR improves the integration tests in two ways: 1. Make the integration tests more reliable and thus less flakey 2. Make the integration tests faster (by introducing concurrency) Tried a lot of different things to make sure that these tests are fast and stable. --- The biggest issue we noticed is that some tests are flakey, these are tests with long running dev-mode processes where watchers are being used and/or dev servers are created. To solve this, all the tests that spawn a process look at stdout/stderr and wait for a message from the process to know whether we can start making changes. For example, in case of an Astro project, you get a `watching for file changes` message. In case of Nuxt project you can wait for an `server warmed up in` and in case of Next.js there is a `Ready in` message. These depend on the tools being used, so this is hardcoded per test instead of a magically automatic solution. These messages allow us to wait until all the initial necessary work, internal watchers and/or dev servers are setup before we start making changes to the files and/or request CSS stylesheets before the server(s) are ready. --- Another improvement is how we setup the dev servers. Before, we used to try and get a free port on the system and use a `--port` flag or a `PORT` environment variable. Instead of doing this (which is slow), we rely on the process itself to show a URL with a port. Basically all tools will try to find a free port if the default port is in use. We can then use the stdout/stderr messages to get the URL and the port to use. To reduce the amount of potential conflicts in ports, we used to run every test and every file sequentially to basically guarantee that ports are free. With this new approach where we rely on the process, I noticed that we don't really run into this issue again (I reran the tests multiple times and they were always stable) <img width="316" alt="image" src="https://github.com/user-attachments/assets/b75ddab4-f919-4995-85d0-f212b603e5c2" /> Note: these tests run Linux, Windows and macOS in this branch just for testing purposes. Once this is done, we will only run Linux tests on PRs and run all 3 of them on the `next` branch. We do make the tests concurrent by default now, which in theory means that there could be conflicts (which in practice means that the process has to do a few more tries to find a free port). To reduce these conflicts, we split up the integration tests such that Vite, PostCSS, CLI, … tests all run in a separate job in the GitHub actions workflow. <img width="312" alt="image" src="https://github.com/user-attachments/assets/fe9a58a1-98eb-4d9b-8845-a7c8a7af5766" /> Comparing this branch against the `next` branch, this is what CI looks like right now: | `next` | `feat/improve-integration-tests` | | --- | --- | | <img width="594" alt="image" src="https://github.com/user-attachments/assets/540d21eb-ab03-42e8-9f6f-b3a071fc7635" /> | <img width="672" alt="image" src="https://github.com/user-attachments/assets/8ef2e891-08a1-464b-9954-4153174ebce7" /> | There also was a point in time where I introduced sequential tests such that all spawned processes still run after each other, but so far I didn't run into issues if we keep them concurrent so I dropped that code. Some small changes I made to make things more reliable: 1. When relying on stdout/stderr messages, we split lines on `\n` and we strip all the ANSI escapes which allows us to not worry about special ANSI characters when finding the URL or a specific message to wait for. 2. Once a test is done, we `child.kill()` the spawned process. If that doesn't work, for whatever reason, we run a `child.kill('SIGKILL')` to force kill the process. This could technically lead to some memory or files not being cleaned up properly, but once CI is done, everything is thrown away anyway. 3. As you can see in the screenshots, I used some nicer names for the workflows. | `next` | `feat/improve-integration-tests` | | --- | --- | | <img width="276" alt="image" src="https://github.com/user-attachments/assets/e574bb53-e21b-4619-9cdb-515431b255b9" /> | <img width="179" alt="image" src="https://github.com/user-attachments/assets/8bc75119-fb91-4500-a1d0-bd09f74c93ad" /> | They also look a bit nicer in the PR overview as well: <img width="929" alt="image" src="https://github.com/user-attachments/assets/04fc71fc-74b0-4e7c-9047-2aada664efef" /> The very last commit just filters out Windows and macOS tests again for PRs (but they are executed on the `next` branch. --- ### Nest steps I think for now we are in a pretty good state, but there are some things we can do to further improve everything (mainly make things faster) but aren't necessary. I also ran into issue while trying it so there is more work to do. 1. More splits — instead of having a Vite folder and PostCSS folder, we can go a step further and have folders for Next.js, Astro, Nuxt, Remix, … 2. Caching — right now we have to run the build step for every OS on every "job". We can re-use the work here by introducing a setup job that the other jobs rely on. @thecrypticace and I tried it already, but were running into some Bun specific Standalone CLI issues when doing that. 3. Remote caching — we could re-enable remote caching such that the `build` step can be full turbo (e.g.: after a PR is merged in `next` and we run everything again)
1 parent bcf7099 commit 2a29c29

27 files changed

+4845
-2514
lines changed

‎.github/workflows/ci.yml‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,11 +77,6 @@ jobs:
7777
- name: Test
7878
run: pnpm run test
7979

80-
- name: Integration Tests
81-
run: pnpm run test:integrations
82-
env:
83-
GITHUB_WORKSPACE: ${{ github.workspace }}
84-
8580
- name: Install Playwright Browsers
8681
run: npx playwright install --with-deps
8782

‎.github/workflows/integration-tests.yml‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
name: Integration Tests
2+
3+
on:
4+
push:
5+
branches: [next]
6+
pull_request:
7+
8+
permissions:
9+
contents: read
10+
11+
jobs:
12+
tests:
13+
strategy:
14+
fail-fast: false
15+
matrix:
16+
node-version: [20]
17+
18+
runner:
19+
- name: Windows
20+
os: windows-latest
21+
22+
- name: Linux
23+
os: namespace-profile-default
24+
25+
- name: macOS
26+
os: macos-14
27+
28+
integration:
29+
- upgrade
30+
- vite
31+
- cli
32+
- postcss
33+
34+
# Exclude windows and macos from being built on feature branches
35+
on-next-branch:
36+
- ${{ github.ref == 'refs/heads/next' }}
37+
exclude:
38+
- on-next-branch: false
39+
runner:
40+
name: Windows
41+
- on-next-branch: false
42+
runner:
43+
name: macOS
44+
45+
runs-on: ${{ matrix.runner.os }}
46+
timeout-minutes: 30
47+
48+
name: ${{ matrix.runner.name }} / ${{ matrix.integration }}
49+
50+
steps:
51+
- uses: actions/checkout@v4
52+
- uses: pnpm/action-setup@v4
53+
54+
- name: Use Node.js ${{ matrix.node-version }}
55+
uses: actions/setup-node@v4
56+
with:
57+
node-version: ${{ matrix.node-version }}
58+
cache: 'pnpm'
59+
60+
# Cargo already skips downloading dependencies if they already exist
61+
- name: Cache cargo
62+
uses: actions/cache@v4
63+
with:
64+
path: |
65+
~/.cargo/bin/
66+
~/.cargo/registry/index/
67+
~/.cargo/registry/cache/
68+
~/.cargo/git/db/
69+
target/
70+
key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
71+
72+
# Cache the `oxide` Rust build
73+
- name: Cache oxide build
74+
uses: actions/cache@v4
75+
with:
76+
path: |
77+
./target/
78+
./crates/node/*.node
79+
./crates/node/index.js
80+
./crates/node/index.d.ts
81+
key: ${{ runner.os }}-oxide-${{ hashFiles('./crates/**/*') }}
82+
83+
- name: Install dependencies
84+
run: pnpm install
85+
86+
- name: Build
87+
run: pnpm run build
88+
env:
89+
CARGO_PROFILE_RELEASE_LTO: 'off'
90+
CARGO_TARGET_X86_64_PC_WINDOWS_MSVC_LINKER: 'lld-link'
91+
92+
- name: Test ${{ matrix.integration }}
93+
run: pnpm run test:integrations ./integrations/${{ matrix.integration }}
94+
env:
95+
GITHUB_WORKSPACE: ${{ github.workspace }}
96+
97+
- name: Notify Discord
98+
if: failure() && github.ref == 'refs/heads/next'
99+
uses: discord-actions/message@v2
100+
with:
101+
webhookUrl: ${{ secrets.DISCORD_WEBHOOK_URL }}
102+
message: 'The [most recent build](<${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}>) on the `next` branch has failed.'

‎integrations/cli/config.test.ts‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,10 @@ test(
161161
},
162162
},
163163
async ({ fs, spawn }) => {
164-
await spawn('pnpm tailwindcss --input src/index.css --output dist/out.css --watch')
164+
let process = await spawn(
165+
'pnpm tailwindcss --input src/index.css --output dist/out.css --watch',
166+
)
167+
await process.onStderr((m) => m.includes('Done in'))
165168

166169
await fs.expectFileToContain('dist/out.css', [
167170
//
@@ -214,7 +217,10 @@ test(
214217
},
215218
},
216219
async ({ fs, spawn }) => {
217-
await spawn('pnpm tailwindcss --input src/index.css --output dist/out.css --watch')
220+
let process = await spawn(
221+
'pnpm tailwindcss --input src/index.css --output dist/out.css --watch',
222+
)
223+
await process.onStderr((m) => m.includes('Done in'))
218224

219225
await fs.expectFileToContain('dist/out.css', [
220226
//
@@ -267,7 +273,10 @@ test(
267273
},
268274
},
269275
async ({ fs, spawn }) => {
270-
await spawn('pnpm tailwindcss --input src/index.css --output dist/out.css --watch')
276+
let process = await spawn(
277+
'pnpm tailwindcss --input src/index.css --output dist/out.css --watch',
278+
)
279+
await process.onStderr((m) => m.includes('Done in'))
271280

272281
await fs.expectFileToContain('dist/out.css', [
273282
//

‎integrations/cli/index.test.ts‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import dedent from 'dedent'
22
import os from 'node:os'
33
import path from 'node:path'
4-
import { describe, expect } from 'vitest'
4+
import { describe } from 'vitest'
55
import { candidate, css, html, js, json, test, ts, yaml } from '../utils'
66

77
const STANDALONE_BINARY = (() => {
@@ -156,9 +156,10 @@ describe.each([
156156
},
157157
},
158158
async ({ root, fs, spawn }) => {
159-
await spawn(`${command} --input src/index.css --output dist/out.css --watch`, {
159+
let process = await spawn(`${command} --input src/index.css --output dist/out.css --watch`, {
160160
cwd: path.join(root, 'project-a'),
161161
})
162+
await process.onStderr((m) => m.includes('Done in'))
162163

163164
await fs.expectFileToContain('project-a/dist/out.css', [
164165
candidate`underline`,
@@ -491,7 +492,7 @@ test(
491492
'pages/nested/foo.jsx': 'content-["pages/nested/foo.jsx"] content-["BAD"]',
492493
},
493494
},
494-
async ({ fs, exec }) => {
495+
async ({ fs, exec, expect }) => {
495496
await exec('pnpm tailwindcss --input index.css --output dist/out.css')
496497

497498
expect(await fs.dumpFiles('./dist/*.css')).toMatchInlineSnapshot(`
@@ -722,7 +723,7 @@ test(
722723
></div>`,
723724
},
724725
},
725-
async ({ fs, exec, spawn, root }) => {
726+
async ({ fs, exec, spawn, root, expect }) => {
726727
await exec('pnpm tailwindcss --input src/index.css --output dist/out.css', {
727728
cwd: path.join(root, 'project-a'),
728729
})
@@ -790,9 +791,13 @@ test(
790791
`)
791792

792793
// Watch mode tests
793-
await spawn('pnpm tailwindcss --input src/index.css --output dist/out.css --watch', {
794-
cwd: path.join(root, 'project-a'),
795-
})
794+
let process = await spawn(
795+
'pnpm tailwindcss --input src/index.css --output dist/out.css --watch',
796+
{
797+
cwd: path.join(root, 'project-a'),
798+
},
799+
)
800+
await process.onStderr((m) => m.includes('Done in'))
796801

797802
// Changes to project-a should not be included in the output, we changed the
798803
// base folder to project-b.
@@ -962,7 +967,7 @@ test(
962967
'pages/nested/foo.jsx': 'content-["pages/nested/foo.jsx"] content-["BAD"]',
963968
},
964969
},
965-
async ({ fs, exec }) => {
970+
async ({ fs, exec, expect }) => {
966971
await exec('pnpm tailwindcss --input index.css --output dist/out.css')
967972

968973
expect(await fs.dumpFiles('./dist/*.css')).toMatchInlineSnapshot(`

‎integrations/package.json‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
"private": true,
55
"devDependencies": {
66
"dedent": "1.5.3",
7-
"fast-glob": "^3.3.2",
8-
"kill-port": "^2.0.1"
7+
"fast-glob": "^3.3.2"
98
}
109
}

‎integrations/postcss/config.test.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,8 @@ test(
148148
},
149149
},
150150
async ({ fs, spawn }) => {
151-
await spawn('pnpm postcss src/index.css --output dist/out.css --watch --verbose')
151+
let process = await spawn('pnpm postcss src/index.css --output dist/out.css --watch --verbose')
152+
await process.onStderr((m) => m.includes('Waiting for file changes'))
152153

153154
await fs.expectFileToContain('dist/out.css', [
154155
//
@@ -218,7 +219,8 @@ test(
218219
},
219220
},
220221
async ({ fs, spawn }) => {
221-
await spawn('pnpm postcss src/index.css --output dist/out.css --watch --verbose')
222+
let process = await spawn('pnpm postcss src/index.css --output dist/out.css --watch --verbose')
223+
await process.onStderr((m) => m.includes('Waiting for file changes'))
222224

223225
await fs.expectFileToContain('dist/out.css', [
224226
//

‎integrations/postcss/core-as-postcss-plugin.test.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { expect } from 'vitest'
1+
import { describe } from 'vitest'
22
import { css, js, json, test } from '../utils'
33

44
const variantConfig = {
@@ -29,9 +29,9 @@ const variantConfig = {
2929
},
3030
}
3131

32-
for (let variant of Object.keys(variantConfig)) {
32+
describe.each(Object.keys(variantConfig))('%s', (variant) => {
3333
test(
34-
`can not use \`tailwindcss\` as a postcss module (${variant})`,
34+
`can not use \`tailwindcss\` as a postcss module`,
3535
{
3636
fs: {
3737
...variantConfig[variant],
@@ -47,12 +47,12 @@ for (let variant of Object.keys(variantConfig)) {
4747
'src/index.css': css`@import 'tailwindcss';`,
4848
},
4949
},
50-
async ({ exec }) => {
50+
async ({ exec, expect }) => {
5151
expect(
5252
exec('pnpm postcss src/index.css --output dist/out.css', undefined, { ignoreStdErr: true }),
5353
).rejects.toThrowError(
5454
`It looks like you're trying to use \`tailwindcss\` directly as a PostCSS plugin. The PostCSS plugin has moved to a separate package, so to continue using Tailwind CSS with PostCSS you'll need to install \`@tailwindcss/postcss\` and update your PostCSS configuration.`,
5555
)
5656
},
5757
)
58-
}
58+
})

‎integrations/postcss/index.test.ts‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import dedent from 'dedent'
22
import path from 'node:path'
3-
import { expect } from 'vitest'
43
import { candidate, css, html, js, json, test, ts, yaml } from '../utils'
54

65
test(
@@ -724,7 +723,7 @@ test(
724723
'pages/nested/foo.jsx': 'content-["pages/nested/foo.jsx"] content-["BAD"]',
725724
},
726725
},
727-
async ({ fs, exec }) => {
726+
async ({ fs, exec, expect }) => {
728727
await exec('pnpm postcss index.css --output dist/out.css')
729728

730729
expect(await fs.dumpFiles('./dist/*.css')).toMatchInlineSnapshot(`
@@ -954,7 +953,7 @@ test(
954953
`,
955954
},
956955
},
957-
async ({ fs, exec, spawn, root }) => {
956+
async ({ fs, exec, spawn, root, expect }) => {
958957
await exec('pnpm postcss src/index.css --output dist/out.css --verbose', {
959958
cwd: path.join(root, 'project-a'),
960959
})
@@ -1020,7 +1019,6 @@ test(
10201019
cwd: path.join(root, 'project-a'),
10211020
},
10221021
)
1023-
10241022
await process.onStderr((message) => message.includes('Waiting for file changes...'))
10251023

10261024
// Changes to project-a should not be included in the output, we changed the
@@ -1215,7 +1213,7 @@ test(
12151213
'pages/nested/foo.jsx': 'content-["pages/nested/foo.jsx"] content-["BAD"]',
12161214
},
12171215
},
1218-
async ({ fs, exec }) => {
1216+
async ({ fs, exec, expect }) => {
12191217
await exec('pnpm postcss index.css --output dist/out.css')
12201218

12211219
expect(await fs.dumpFiles('./dist/*.css')).toMatchInlineSnapshot(`

‎integrations/postcss/next.test.ts‎

Lines changed: 28 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { expect } from 'vitest'
1+
import { describe } from 'vitest'
22
import { candidate, css, fetchStyles, js, json, retryAssertion, test } from '../utils'
33

44
test(
@@ -56,7 +56,7 @@ test(
5656
`,
5757
},
5858
},
59-
async ({ fs, exec }) => {
59+
async ({ fs, exec, expect }) => {
6060
await exec('pnpm next build')
6161

6262
let files = await fs.glob('.next/static/css/**/*.css')
@@ -70,9 +70,10 @@ test(
7070
])
7171
},
7272
)
73-
;['turbo', 'webpack'].forEach((bundler) => {
73+
74+
describe.each(['turbo', 'webpack'])('%s', (bundler) => {
7475
test(
75-
`dev mode (${bundler})`,
76+
'dev mode',
7677
{
7778
fs: {
7879
'package.json': json`
@@ -126,26 +127,35 @@ test(
126127
`,
127128
},
128129
},
129-
async ({ fs, spawn, getFreePort }) => {
130-
let port = await getFreePort()
131-
await spawn(`pnpm next dev ${bundler === 'turbo' ? '--turbo' : ''} --port ${port}`)
130+
async ({ fs, spawn, expect }) => {
131+
let process = await spawn(`pnpm next dev ${bundler === 'turbo' ? '--turbo' : ''}`)
132+
133+
let url = ''
134+
await process.onStdout((m) => {
135+
let match = /Local:\s*(http.*)/.exec(m)
136+
if (match) url = match[1]
137+
return Boolean(url)
138+
})
139+
140+
await process.onStdout((m) => m.includes('Ready in'))
132141

133142
await retryAssertion(async () => {
134-
let css = await fetchStyles(port)
143+
let css = await fetchStyles(url)
135144
expect(css).toContain(candidate`underline`)
136145
})
137146

138-
await retryAssertion(async () => {
139-
await fs.write(
140-
'app/page.js',
141-
js`
142-
export default function Page() {
143-
return <h1 className="underline text-red-500">Hello, Next.js!</h1>
144-
}
145-
`,
146-
)
147+
await fs.write(
148+
'app/page.js',
149+
js`
150+
export default function Page() {
151+
return <h1 className="underline text-red-500">Hello, Next.js!</h1>
152+
}
153+
`,
154+
)
155+
await process.onStdout((m) => m.includes('Compiled in'))
147156

148-
let css = await fetchStyles(port)
157+
await retryAssertion(async () => {
158+
let css = await fetchStyles(url)
149159
expect(css).toContain(candidate`underline`)
150160
expect(css).toContain(candidate`text-red-500`)
151161
})

0 commit comments

Comments
 (0)