Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ site/src/**/*.md
.verdaccio-storage
.eslintcache
_test_out/**
tests/unit/utils/tmp
*.crt
*.key

Expand Down
2 changes: 1 addition & 1 deletion src/utils/telemetry/report-error.ts
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
11 changes: 7 additions & 4 deletions tests/integration/commands/dev/dev.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -382,14 +382,15 @@ describe.concurrent('command/dev', () => {
t.expect(nodeVer).toBeGreaterThanOrEqual(18)

await withSiteBuilder(t, async (builder) => {
const targetPort = await getPort()
const externalServer = startExternalServer({
host: '127.0.0.1',
port: 4567,
port: targetPort,
})
await builder.build()

await withDevServer(
{ cwd: builder.directory, command: 'node', framework: '#custom', targetPort: 4567, skipWaitPort: true },
{ cwd: builder.directory, command: 'node', framework: '#custom', targetPort, skipWaitPort: true },
async (server) => {
const response = await fetch(`${server.url}/test`)
t.expect(response.status).toBe(200)
Expand Down Expand Up @@ -828,13 +829,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 +892,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
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 } },
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Env value should be a string for consistency and to match NodeJS.ProcessEnv typing.

Line 122 of this same file uses NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: '1'. Node coerces numeric values at spawn time, but TS's ProcessEnv type expects strings and downstream code typically does strict equality against '1'.

-      devServer: { env: { NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: 1 } },
+      devServer: { env: { NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: '1' } },
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
devServer: { env: { NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: 1 } },
devServer: { env: { NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: '1' } },
🤖 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` at line 41, The env value
for NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS is currently a number; change it to a
string to match NodeJS.ProcessEnv and existing tests (use '1' instead of 1).
Locate the devServer config object (devServer: { env: {
NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: ... } }) in the test and update the
value to '1' so it matches the usage on line with
NETLIFY_DEV_SERVER_CHECK_SSG_ENDPOINTS: '1' and any strict equality checks
elsewhere.

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 +55
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fragile string replace — silently no-ops if targetPort = 6123 is ever reformatted.

String.prototype.replace with a literal needle returns the original string unchanged when no match is found, so a future reformat (e.g., targetPort=6123, different whitespace, or bumping the placeholder value) would leave netlify.toml pointing at the old port while next dev listens on the dynamic one — and the failure mode is a mysterious timeout rather than a clear error.

🛠️ Suggested fix: assert the replacement happened (or use a regex with a post-check)
-        await writeFile(
-          netlifyTomlPath,
-          (await readFile(netlifyTomlPath, 'utf8')).replace('targetPort = 6123', `targetPort = ${targetPort.toString()}`),
-        )
+        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 substitute targetPort in ${netlifyTomlPath}`)
+        }
+        await writeFile(netlifyTomlPath, updatedToml)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await writeFile(
netlifyTomlPath,
(await readFile(netlifyTomlPath, 'utf8')).replace('targetPort = 6123', `targetPort = ${targetPort.toString()}`),
)
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 substitute targetPort in ${netlifyTomlPath}`)
}
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 test uses a fragile literal string replace on the netlify toml contents
via readFile/netlifyTomlPath and writeFile which will silently no-op if
formatting changes; update the logic in the test to perform the replacement with
a regex (matching variations like whitespace around `targetPort` and the value)
or run the replace and then assert that the result differs from the original
(using targetPort.toString() as the new value), and if no change occurred
throw/assert a failure before calling writeFile so the test fails loudly rather
than leaving netlify.toml unchanged.

},
},
() => {
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')
})
},
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.

test('should not check the endpoint existence for hidden proxies', async (t) => {
await withSiteBuilder(t, async (builder) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,18 @@ import {
createAIGatewayTestData,
} from '../../utils/ai-gateway-helpers.js'

const DEFAULT_PORT = 9999
const SERVE_TIMEOUT = 180_000

const withFunctionsServer = async (
{
args = [],
builder,
port = DEFAULT_PORT,
port,
env = {},
}: {
args?: string[]
builder: SiteBuilder
port?: number
port: number
env?: NodeJS.ProcessEnv
},
testHandler: () => Promise<unknown>,
Expand Down Expand Up @@ -74,8 +73,9 @@ describe.concurrent('functions:serve command', () => {
})
.build()

await withFunctionsServer({ builder }, async () => {
const response = await fetch(`http://localhost:9999/.netlify/functions/ping`)
const port = await getPort()
await withFunctionsServer({ builder, args: ['--port', port.toString()], port }, async () => {
const response = await fetch(`http://localhost:${port.toString()}/.netlify/functions/ping`)
t.expect(await response.text()).toEqual('ping')
})
})
Expand Down
19 changes: 13 additions & 6 deletions tests/integration/framework-detection.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import execa from 'execa'
Comment thread
coderabbitai[bot] marked this conversation as resolved.
import getPort from 'get-port'
import fetch from 'node-fetch'
import { describe, test } from 'vitest'

Expand Down Expand Up @@ -89,6 +90,7 @@ describe.concurrent('frameworks/framework-detection', () => {

test('should warn if using static server and `targetPort` is configured', async (t) => {
await withSiteBuilder(t, async (builder) => {
const targetPort = await getPort()
await builder
.withContentFile({
path: 'public/index.html',
Expand All @@ -97,7 +99,7 @@ describe.concurrent('frameworks/framework-detection', () => {
.build()

await withDevServer(
{ cwd: builder.directory, args: ['--dir', 'public', '--target-port', '3000'] },
{ cwd: builder.directory, args: ['--dir', 'public', '--target-port', targetPort.toString()] },
async ({ output, url }) => {
const response = await fetch(url)
const responseContent = await response.text()
Expand All @@ -111,11 +113,12 @@ describe.concurrent('frameworks/framework-detection', () => {

test('should run `command` when both `command` and `targetPort` are configured', async (t) => {
await withSiteBuilder(t, async (builder) => {
const targetPort = await getPort()
await builder.withNetlifyToml({ config: { build: { publish: 'public' } } }).build()

try {
await withDevServer(
{ cwd: builder.directory, args: ['--command', 'echo hello', '--target-port', '3000'] },
{ cwd: builder.directory, args: ['--command', 'echo hello', '--target-port', targetPort.toString()] },
async () => {},
true,
)
Expand Down Expand Up @@ -185,10 +188,11 @@ describe.concurrent('frameworks/framework-detection', () => {

test('should throw if framework=#custom but command is missing', async (t) => {
await withSiteBuilder(t, async (builder) => {
const targetPort = await getPort()
await builder.withNetlifyToml({ config: { dev: { framework: '#custom' } } }).build()

try {
await withDevServer({ cwd: builder.directory, args: ['--target-port', '3000'] }, async () => {}, true)
await withDevServer({ cwd: builder.directory, args: ['--target-port', targetPort.toString()] }, async () => {}, true)
t.expect.unreachable()
} catch (err) {
t.expect(err).toHaveProperty('stdout')
Expand Down Expand Up @@ -217,11 +221,12 @@ describe.concurrent('frameworks/framework-detection', () => {

test('should start custom command if framework=#custom, command and targetPort are configured', async (t) => {
await withSiteBuilder(t, async (builder) => {
const targetPort = await getPort()
await builder.withNetlifyToml({ config: { dev: { framework: '#custom', publish: 'public' } } }).build()

try {
await withDevServer(
{ cwd: builder.directory, args: ['--command', 'echo hello', '--target-port', '3000'] },
{ cwd: builder.directory, args: ['--command', 'echo hello', '--target-port', targetPort.toString()] },
async () => {},
true,
)
Expand All @@ -237,6 +242,7 @@ describe.concurrent('frameworks/framework-detection', () => {

test(`should print specific error when command doesn't exist`, async (t) => {
await withSiteBuilder(t, async (builder) => {
const targetPort = await getPort()
await builder.build()

try {
Expand All @@ -247,7 +253,7 @@ describe.concurrent('frameworks/framework-detection', () => {
'--command',
'oops-i-did-it-again forgot-to-use-a-valid-command',
'--target-port',
'3000',
targetPort.toString(),
'--framework',
'#custom',
],
Expand Down Expand Up @@ -345,11 +351,12 @@ describe.concurrent('frameworks/framework-detection', () => {

test('should not run framework detection if command and targetPort are configured', async (t) => {
await withSiteBuilder(t, async (builder) => {
const targetPort = await getPort()
await builder.withContentFile({ path: 'config.toml', content: '' }).build()

try {
await withDevServer(
{ cwd: builder.directory, args: ['--command', 'echo hello', '--target-port', '3000'] },
{ cwd: builder.directory, args: ['--command', 'echo hello', '--target-port', targetPort.toString()] },
async () => {},
true,
)
Expand Down
23 changes: 6 additions & 17 deletions tests/unit/utils/copy-template-dir/copy-template-dir.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,19 @@ import path from 'path'
import { fileURLToPath } from 'url'

import { readdirpPromise } from 'readdirp'
import { describe, expect, test } from 'vitest'
import { afterEach, describe, expect, test } from 'vitest'

import { copyTemplateDir } from '../../../../src/utils/copy-template-dir/copy-template-dir.js'

const __filename = fileURLToPath(import.meta.url)
const __dirname = path.dirname(__filename)
const outDir = path.join(__dirname, '../tmp')

describe('copyTemplateDir', () => {
afterEach(() => {
fs.rmSync(outDir, { recursive: true, force: true })
})

test('should write a bunch of files', async () => {
const checkCreatedFileNames = (names: string[]) => {
expect(names).toContain('.a')
Expand All @@ -25,7 +30,6 @@ describe('copyTemplateDir', () => {
}

const inDir = path.join(__dirname, 'fixtures')
const outDir = path.join(__dirname, '../tmp')

const createdFiles = await copyTemplateDir(inDir, outDir, {})

Expand All @@ -38,45 +42,30 @@ describe('copyTemplateDir', () => {
// Checks that the files were created in the file system
const files = await readdirpPromise(outDir)
checkCreatedFileNames(files.map((file) => file.path))

// Cleanup
fs.rmdirSync(outDir, { recursive: true })
})

test('should inject context variables strings', async () => {
const inDir = path.join(__dirname, 'fixtures')
const outDir = path.join(__dirname, '../tmp')

await copyTemplateDir(inDir, outDir, { foo: 'bar' })

const fileContent = fs.readFileSync(path.join(outDir, '1.txt'), 'utf-8').trim()
expect(fileContent).toBe('hello bar sama')

// Cleanup
fs.rmdirSync(outDir, { recursive: true })
})

test('should inject context variables strings into filenames', async () => {
const inDir = path.join(__dirname, 'fixtures')
const outDir = path.join(__dirname, '../tmp')

await copyTemplateDir(inDir, outDir, { foo: 'bar' })

expect(fs.existsSync(path.join(outDir, 'bar.txt'))).toBe(true)

// Cleanup
fs.rmdirSync(outDir, { recursive: true })
})

test('should inject context variables strings into directory names', async () => {
const inDir = path.join(__dirname, 'fixtures')
const outDir = path.join(__dirname, '../tmp')

await copyTemplateDir(inDir, outDir, { foo: 'bar' })

expect(fs.existsSync(path.join(outDir, 'bar'))).toBe(true)

// Cleanup
fs.rmdirSync(outDir, { recursive: true })
})
})
Loading