Skip to content

fix: stabilize integration test suite #8208

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

Open
jherr wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: stabilize integration test suite #8208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 10 additions & 1 deletion src/commands/completion/completion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { fileURLToPath } from 'url'
import inquirer from 'inquirer'

import type { OptionValues } from 'commander'
import { install, uninstall } from '@pnpm/tabtab'
import { install, isShellSupported, uninstall } from '@pnpm/tabtab'

import { generateAutocompletion } from '../../lib/completion/index.js'
import {
Expand All @@ -27,10 +27,19 @@ export const completionGenerate = async (_options: OptionValues, command: BaseCo
return logAndThrowError(`There has been an error generating the completion script.`)
}

let shell: 'bash' | 'fish' | 'pwsh' | 'zsh' | undefined
if (typeof _options.shell === 'string') {
if (!isShellSupported(_options.shell)) {
return logAndThrowError(`Unsupported shell "${_options.shell}". Supported shells are bash, fish, pwsh, and zsh.`)
}
shell = _options.shell
}

generateAutocompletion(parent)
await install({
name: parent.name(),
completer,
shell,
})

const completionScriptPath = join(homedir(), `.config/tabtab/${parent.name()}.zsh`)
Expand Down
1 change: 1 addition & 0 deletions src/commands/completion/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export const createCompletionCommand = (program: BaseCommand) => {
.command('completion:install')
.alias('completion:generate')
.description('Generates completion script for your preferred shell')
.option('--shell <shell>', 'Shell to install completion for (bash, fish, pwsh, zsh)')
.action(async (options: OptionValues, command: BaseCommand) => {
const { completionGenerate } = await import('./completion.js')
await completionGenerate(options, command)
Expand Down
2 changes: 1 addition & 1 deletion src/utils/telemetry/report-error.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const dirPath = dirname(fileURLToPath(import.meta.url))
*/
// @ts-expect-error TS(7006) FIXME: Parameter 'error' implicitly has an 'any' type.
export const reportError = async function (error, config = {}) {
if (isCI) {
if (isCI || process.env.CI) {
return
}
// convert a NotifiableError to an error class
Expand Down
16 changes: 4 additions & 12 deletions tests/integration/commands/completion/completion-install.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { describe, test, beforeAll, afterAll } from 'vitest'
import fs from 'fs'
import { rm } from 'fs/promises'
import { handleQuestions, CONFIRM, DOWN, NO, answerWithValue } from '../../utils/handle-questions.js'
import { handleQuestions, CONFIRM, NO } from '../../utils/handle-questions.js'
import execa from 'execa'
import { cliPath } from '../../utils/cli-path.js'
import { join } from 'path'
Expand All @@ -27,13 +27,9 @@ describe('completion:install command', () => {
'should add compinit to .zshrc when user confirms prompt',
async ({ expect }) => {
fs.writeFileSync(zshConfigPath, TABTAB_CONFIG_LINE)
const childProcess = execa(cliPath, ['completion:install'], options)
const childProcess = execa(cliPath, ['completion:install', '--shell', 'zsh'], options)

handleQuestions(childProcess, [
{
question: 'Which Shell do you use ?',
answer: answerWithValue(DOWN),
},
{
question: 'We will install completion to ~/.zshrc, is it ok ?',
answer: CONFIRM,
Expand All @@ -54,20 +50,16 @@ describe('completion:install command', () => {
'should not add compinit to .zshrc when user does not confirm prompt',
async ({ expect }) => {
fs.writeFileSync(zshConfigPath, TABTAB_CONFIG_LINE)
const childProcess = execa(cliPath, ['completion:install'], options)
const childProcess = execa(cliPath, ['completion:install', '--shell', 'zsh'], options)

handleQuestions(childProcess, [
{
question: 'Which Shell do you use ?',
answer: answerWithValue(DOWN),
},
{
question: 'We will install completion to ~/.zshrc, is it ok ?',
answer: CONFIRM,
},
{
question: 'Would you like to add it?',
answer: answerWithValue(NO),
answer: [NO, CONFIRM],
},
])

Expand Down
6 changes: 4 additions & 2 deletions tests/integration/commands/dev/dev.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -828,13 +828,15 @@ describe.concurrent('command/dev', () => {

test('deploy environment variables injected by onDev plugin hooks are injected into functions', async (t) => {
await withSiteBuilder(t, async (builder) => {
const targetPort = await getPort()

await builder
.withNetlifyToml({
config: {
plugins: [{ package: './plugins/plugin' }],
dev: {
command: 'node index.mjs',
targetPort: 4445,
targetPort,
},
},
})
Expand Down Expand Up @@ -889,7 +891,7 @@ describe.concurrent('command/dev', () => {
res.end();
})

server.listen(4445)
server.listen(${targetPort.toString()})
`,
})
.build()
Expand Down
64 changes: 43 additions & 21 deletions tests/integration/commands/dev/redirects.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import { readFile, writeFile } from 'fs/promises'
import { join } from 'path'

import getPort from 'get-port'
import fetch from 'node-fetch'
import { describe, expect, test } from 'vitest'
Expand Down Expand Up @@ -32,27 +35,46 @@ describe('redirects', async () => {
})
})

await setupFixtureTests('next-app', { devServer: { env: { NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: 1 } } }, () => {
test<FixtureTestContext>('should prefer local files instead of redirect when not forced', async ({ devServer }) => {
const response = await fetch(`http://localhost:${devServer!.port}/test.txt`, {})

expect(response.status).toBe(200)

const result = await response.text()
expect(result.trim()).toEqual('hello world')
})

test<FixtureTestContext>('should check for the dynamic page existence before doing redirect', async ({
devServer,
}) => {
const response = await fetch(`http://localhost:${devServer!.port}/`, {})

expect(response.status).toBe(200)

const result = await response.text()
expect(result.toLowerCase()).not.toContain('netlify')
})
})
await setupFixtureTests(
'next-app',
{
devServer: { env: { NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: 1 } },
setup: async ({ fixture }) => {
const targetPort = await getPort()
const packageJsonPath = join(fixture.directory, 'package.json')
const netlifyTomlPath = join(fixture.directory, 'netlify.toml')
const packageJson = JSON.parse(await readFile(packageJsonPath, 'utf8')) as { scripts: { dev: string } }

packageJson.scripts.dev = `next dev -p ${targetPort.toString()}`
await writeFile(packageJsonPath, `${JSON.stringify(packageJson, null, 2)}\n`)
await writeFile(
netlifyTomlPath,
(await readFile(netlifyTomlPath, 'utf8')).replace('targetPort = 6123', `targetPort = ${targetPort.toString()}`),
)
Comment on lines +50 to +53
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 24, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Silent no-op risk: assert the targetPort replacement actually happened.

String.prototype.replace returns the original string unchanged if 'targetPort = 6123' isn't present (e.g., the fixture's default port changes, someone reformats to targetPort=6123, or the line is moved/removed). The test would then silently fall back to whatever port is checked into the fixture, re-introducing a flaky/unexpected port binding without any signal.

Either pattern-match more robustly or assert that the replacement occurred.

🛡️ Proposed fix 
-        packageJson.scripts.dev = `next dev -p ${targetPort.toString()}`
-        await writeFile(packageJsonPath, `${JSON.stringify(packageJson, null, 2)}\n`)
-        await writeFile(
-          netlifyTomlPath,
-          (await readFile(netlifyTomlPath, 'utf8')).replace('targetPort = 6123', `targetPort = ${targetPort.toString()}`),
-        )
+        packageJson.scripts.dev = `next dev -p ${targetPort.toString()}`
+        await writeFile(packageJsonPath, `${JSON.stringify(packageJson, null, 2)}\n`)
+
+        const originalToml = await readFile(netlifyTomlPath, 'utf8')
+        const updatedToml = originalToml.replace(/targetPort\s*=\s*\d+/, `targetPort = ${targetPort.toString()}`)
+        if (updatedToml === originalToml) {
+          throw new Error(`Failed to rewrite targetPort in ${netlifyTomlPath}; fixture may have changed.`)
+        }
+        await writeFile(netlifyTomlPath, updatedToml)
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/integration/commands/dev/redirects.test.ts` around lines 50 - 53, The
current replace call may be a silent no-op if the exact substring 'targetPort =
6123' isn't present; update the logic around readFile/replace/writeFile
(referencing netlifyTomlPath and targetPort) to perform a more robust
regex-based replacement (e.g., allow flexible spacing) and then assert that the
replacement actually occurred by comparing the original and replaced contents or
by checking a replace count; if no replacement happened, throw or fail the test
immediately before calling writeFile so the test fails loudly rather than
silently using the unchanged fixture.

},
},
() => {
test<FixtureTestContext>('should prefer local files instead of redirect when not forced', async ({ devServer }) => {
const response = await fetch(`http://localhost:${devServer!.port}/test.txt`, {})

expect(response.status).toBe(200)

const result = await response.text()
expect(result.trim()).toEqual('hello world')
})

test<FixtureTestContext>('should check for the dynamic page existence before doing redirect', async ({
devServer,
}) => {
const response = await fetch(`http://localhost:${devServer!.port}/`, {})

expect(response.status).toBe(200)

const result = await response.text()
expect(result.toLowerCase()).not.toContain('netlify')
})
},
)

test('should not check the endpoint existence for hidden proxies', async (t) => {
await withSiteBuilder(t, async (builder) => {
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/commands/help/__snapshots__/help.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,10 @@ USAGE
$ netlify completion [options]

OPTIONS
-h, --help display help for command
--debug Print debugging information
--auth <token> Netlify auth token - can be used to run this command
without logging in
-h, --help display help for command

DESCRIPTION
Run this command to see instructions for your shell.
Expand Down
Loading