Skip to content

test: fix test setup #2749

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Feb 12, 2025
Merged

test: fix test setup #2749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
37c5fbd
test: fail prepare fixtures script if fixtures fail to install deps o…
pieh Jan 27, 2025
0d9c1f4
test: intentionally break deps installation / fixture build to test p…
pieh Jan 27, 2025
8294b81
Revert "test: intentionally break deps installation / fixture build t…
pieh Jan 27, 2025
6758465
Merge branch 'main' into test-fail-prepare-script-if-fixture-prep-fails
pieh Jan 30, 2025
f7c9dc8
Merge branch 'main' into test-fail-prepare-script-if-fixture-prep-fails
pieh Feb 3, 2025
e68d575
ci: upgrade corepack
pieh Feb 11, 2025
948fd2b
test: don't prepare e2e-only fixtures ahead of time
pieh Feb 12, 2025
49126e0
test: make global corepack update/installation commands as similar as…
pieh Feb 12, 2025
de940d9
test: remove integration test that was skipped for over a year, we do…
pieh Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions .github/workflows/run-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ jobs:
cache-dependency-path: '**/package-lock.json'
- uses: oven-sh/setup-bun@v1
- name: setup pnpm/yarn
run: corepack enable
run: |
npm install -g corepack
corepack enable
shell: bash
- name: Install Deno
uses: denoland/setup-deno@v1
Expand Down Expand Up @@ -127,8 +129,18 @@ jobs:
node-version: '18.x'
cache: 'npm'
cache-dependency-path: '**/package-lock.json'
- name: Prefer npm global on windows
if: runner.os == 'Windows'
# On Windows by default PATH prefers corepack bundled with Node.js
# This prepends npm global to PATH to ensure that npm installed global corepack is used instead
run: |
echo "$(npm config get prefix)" >> "$GITHUB_PATH"
shell: bash
- name: setup pnpm/yarn
run: corepack enable
run: |
# global corepack installation requires --force on Windows, otherwise EEXIST errors occur
npm install -g corepack --force
Copy link
Contributor

Choose a reason for hiding this comment

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

Learning - why corepack@latest above, and then --force in this step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The @latest selector in the e2e is probably not needed - just result of iterating and trying to find working solution. I will remove it to keep things in sync.

The --force here is needed, because Windows one otherwise would fail to install - without it I was getting following error:

npm error code EEXIST
npm error path C:\npm\prefix\yarn
npm error EEXIST: file already exists
npm error File exists: C:\npm\prefix\yarn
npm error Remove the existing file and try again, or run npm
npm error with --force to overwrite files recklessly.

And we only needed it in jobs that run integration tests because only those use Windows runners. E2E ones are linux only hence don't need it.

corepack enable
shell: bash
- name: Install Deno
uses: denoland/setup-deno@v1
Expand Down
46 changes: 0 additions & 46 deletions tests/integration/turborepo.test.ts
View file
Open in desktop

This file was deleted.

50 changes: 49 additions & 1 deletion tests/prepare.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,30 @@ const NEXT_VERSION = process.env.NEXT_VERSION ?? 'latest'
const fixturesDir = fileURLToPath(new URL(`./fixtures`, import.meta.url))
const fixtureFilter = argv[2] ?? ''

// E2E tests run next builds, so we don't need to prepare those ahead of time for integration tests
const e2eOnlyFixtures = new Set([
'after',
'cli-before-regional-blobs-support',
'dist-dir',
// There is also a bug on Windows on Node.js 18.20.6, that cause build failures on this fixture
// see https://github.com/opennextjs/opennextjs-netlify/actions/runs/13268839161/job/37043172448?pr=2749#step:12:78
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to make this comment resolvable? For example, saying something more along the lines of "when so and so is merged in, we can remove this test from this set." Perhaps a link to a github or linear issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is currently no Next.js issue to link to as of now and I didn't find time to create proper Next.js issue for it myself (and I would need to prep repro and make sure it works on my personal windows machine, so that's not something I really have time to do during work hours :S). Without that issue in Next.js there is not much of a point creating internal linear issue, because it wouldn't say anything more than this comment is saying now.

Being a bit sneaky I could just remove this whole comment as it's still true that we don't actually use this fixture in integration tests and let someone possibly encounter that problem without warning, so adding this comment was just trying to balance providing at least some information here while acknowledging that I don't have bandwidth to create proper tracking ticket in Next.js repository.

'middleware-og',
'middleware-single-matcher',
'nx-integrated',
'turborepo',
'turborepo-npm',
'unstable-cache',
])

const limit = pLimit(Math.max(2, cpus().length / 2))
const fixtures = readdirSync(fixturesDir)
// Ignoring things like `.DS_Store`.
.filter((fixture) => !fixture.startsWith('.'))
// Applying the filter, if one is set.
.filter((fixture) => !fixtureFilter || fixture.startsWith(fixtureFilter))
// Filter out fixtures that are only needed for E2E tests
.filter((fixture) => !e2eOnlyFixtures.has(fixture))

console.log(`🧪 Preparing fixtures: ${fixtures.join(', ')}`)
const fixtureList = new Set(fixtures)
const fixtureCount = fixtures.length
Expand Down Expand Up @@ -62,7 +80,15 @@ const promises = fixtures.map((fixture) =>
this.push(chunk.toString().replace(/\n/gm, `\n[${fixture}] `))
callback()
},
flush(callback) {
// final transform might create non-terminated line with a prefix
// so this is just to make sure we end with a newline so further writes
// to same destination stream start on a new line for better readability
this.push('\n')
callback()
},
})

console.log(`[${fixture}] Running \`${cmd}\`...`)
const output = execaCommand(cmd, {
cwd,
Expand All @@ -80,6 +106,11 @@ const promises = fixtures.map((fixture) =>
operation: 'revert',
})
}
if (output.exitCode !== 0) {
const errorMessage = `[${fixture}] 🚨 Failed to install dependencies or build a fixture`
console.error(errorMessage)
throw new Error(errorMessage)
}
fixtureList.delete(fixture)
})
}).finally(() => {
Expand All @@ -91,5 +122,22 @@ const promises = fixtures.map((fixture) =>
}
}),
)
await Promise.allSettled(promises)
const prepareFixturesResults = await Promise.allSettled(promises)
const failedFixturesErrors = prepareFixturesResults
.map((promise) => {
if (promise.status === 'rejected') {
return promise.reason
}
return null
})
.filter(Boolean)

if (failedFixturesErrors.length > 0) {
console.error('Some fixtures failed to prepare:')
for (const error of failedFixturesErrors) {
console.error(error.message)
}
process.exit(1)
}

console.log('🎉 All fixtures prepared')
Loading