Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
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
56 changes: 16 additions & 40 deletions .github/workflows/build_and_test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,6 @@ jobs:
with:
afterBuild: |
export IS_TURBOPACK_TEST=1
export TURBOPACK_DEV=1
export NEXT_TEST_MODE=dev
export NEXT_TEST_REACT_VERSION="${{ matrix.react }}"

Expand All @@ -259,14 +258,27 @@ jobs:
secrets: inherit

test-turbopack-integration:
name: test turbopack development integration
name: test turbopack integration
needs: ['optimize-ci', 'changes', 'build-next', 'build-native']
if: ${{ needs.optimize-ci.outputs.skip == 'false' && needs.changes.outputs.docs-only == 'false' }}

strategy:
fail-fast: false
matrix:
group: [1/6, 2/6, 3/6, 4/6, 5/6, 6/6]
group:
- 1/13
- 2/13
- 3/13
- 4/13
- 5/13
- 6/13
- 7/13
- 8/13
- 9/13
- 10/13
- 11/13
- 12/13
- 13/13
# Empty value uses default
# TODO: Run with React 18.
# Integration tests use the installed React version in next/package.json.include:
Expand All @@ -278,14 +290,13 @@ jobs:
nodeVersion: 20.9.0
afterBuild: |
export IS_TURBOPACK_TEST=1
export TURBOPACK_DEV=1
export NEXT_TEST_REACT_VERSION="${{ matrix.react }}"

node run-tests.js \
--timings \
-g ${{ matrix.group }} \
--type integration
stepName: 'test-turbopack-integration-react-${{ matrix.react }}-${{ matrix.group }}'
stepName: 'test-turbopack-integration-${{ matrix.group }}-react-${{ matrix.react }}'
secrets: inherit

test-turbopack-production:
Expand All @@ -311,37 +322,13 @@ jobs:
nodeVersion: 20.9.0
afterBuild: |
export IS_TURBOPACK_TEST=1
export TURBOPACK_BUILD=1
export NEXT_TEST_MODE=start
export NEXT_TEST_REACT_VERSION="${{ matrix.react }}"

node run-tests.js --timings -g ${{ matrix.group }} --type production
stepName: 'test-turbopack-production-react-${{ matrix.react }}-${{ matrix.group }}'
secrets: inherit

test-turbopack-production-integration:
name: test turbopack production integration
needs: ['optimize-ci', 'changes', 'build-next', 'build-native']
if: ${{ needs.optimize-ci.outputs.skip == 'false' && needs.changes.outputs.docs-only == 'false' }}

strategy:
fail-fast: false
matrix:
group: [1/7, 2/7, 3/7, 4/7, 5/7, 6/7, 7/7]
uses: ./.github/workflows/build_reusable.yml
with:
nodeVersion: 20.9.0
afterBuild: |
export IS_TURBOPACK_TEST=1
export TURBOPACK_BUILD=1

node run-tests.js \
--timings \
-g ${{ matrix.group }} \
--type integration
stepName: 'test-turbopack-production-integration-${{ matrix.group }}'
secrets: inherit

test-rspack-dev:
name: test rspack dev
needs: ['optimize-ci', 'changes', 'build-next', 'build-native']
Expand All @@ -360,10 +347,6 @@ jobs:
export NEXT_RSPACK=1
export NEXT_TEST_USE_RSPACK=1

# HACK: Despite the name, this environment variable is only used to gate
# tests, so it's applicable to rspack
export TURBOPACK_DEV=1

node run-tests.js \
--test-pattern '^(test\/(development|e2e))/.*\.test\.(js|jsx|ts|tsx)$' \
--timings \
Expand All @@ -389,10 +372,6 @@ jobs:
export NEXT_RSPACK=1
export NEXT_TEST_USE_RSPACK=1

# HACK: Despite the name, this environment variable is only used to gate
# tests, so it's applicable to rspack
export TURBOPACK_DEV=1

Copy link
Contributor

Choose a reason for hiding this comment

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

HACK comments explain environment variables that are no longer being set, creating misleading documentation.

View Details
📝 Patch Details
diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml
index e8a65c7efd..eac7d02756 100644
--- a/.github/workflows/build_and_test.yml
+++ b/.github/workflows/build_and_test.yml
@@ -402,9 +402,6 @@ jobs:
         export NEXT_RSPACK=1
         export NEXT_TEST_USE_RSPACK=1
 
-        # HACK: Despite the name, this environment variable is only used to gate
-        # tests, so it's applicable to rspack
-
         node run-tests.js --timings -g ${{ matrix.group }} --type production
       stepName: 'test-rspack-production-react-${{ matrix.react }}-${{ matrix.group }}'
     secrets: inherit
@@ -427,9 +424,6 @@ jobs:
         export NEXT_RSPACK=1
         export NEXT_TEST_USE_RSPACK=1
 
-        # HACK: Despite the name, this environment variable is only used to gate
-        # tests, so it's applicable to rspack
-
         node run-tests.js \
           --timings \
           -g ${{ matrix.group }} \

Analysis

Orphaned HACK Comments in GitHub Workflow

Issue Description

The GitHub workflow file .github/workflows/build_and_test.yml contained misleading HACK comments that referenced non-existent environment variables. These comments were remnants from a previous "cleanup" commit that removed the corresponding export statements but left the explanatory comments behind.

Technical Analysis

Problem Manifestation

Two identical HACK comments existed at lines 405-406 and 430-431:

# HACK: Despite the name, this environment variable is only used to gate
# tests, so it's applicable to rspack

These comments were followed by empty lines and then the test execution commands, with no environment variable exports between them.

Root Cause

Git history analysis revealed that commit 602e541bc2 ("cleanup") removed several environment variable exports including:

  • export TURBOPACK_BUILD=1
  • export TURBOPACK_DEV=1

However, the commit removed the export statements but failed to remove the associated HACK comments that were explaining why these "TURBOPACK_*" variables were applicable to rspack tests despite their names.

Impact

Documentation Confusion: The orphaned comments created misleading inline documentation that referenced non-existent functionality, potentially confusing developers trying to understand the workflow configuration.

Maintenance Overhead: Future maintainers might waste time looking for the environment variables these comments referenced, or incorrectly assume variables were missing when they were intentionally removed.

Code Quality: Stale comments violate clean code principles and indicate incomplete refactoring.

Resolution

Removed both orphaned HACK comment blocks from:

  • Lines 405-406 in the test-rspack-production job
  • Lines 430-431 in the test-rspack-production-integration job

The workflow now flows directly from the rspack environment variable exports (NEXT_RSPACK=1, NEXT_TEST_USE_RSPACK=1) to the test execution commands, eliminating the confusing intermediate comments that referenced nothing.

Verification

  • Confirmed no TURBOPACK_BUILD references exist anywhere in the codebase
  • Verified YAML structure remains valid after comment removal
  • Validated that the workflow maintains its intended functionality without the orphaned documentation

node run-tests.js \
--timings \
-g ${{ matrix.group }} \
Expand Down Expand Up @@ -425,7 +404,6 @@ jobs:

# HACK: Despite the name, this environment variable is only used to gate
# tests, so it's applicable to rspack
export TURBOPACK_BUILD=1

node run-tests.js --timings -g ${{ matrix.group }} --type production
stepName: 'test-rspack-production-react-${{ matrix.react }}-${{ matrix.group }}'
Expand All @@ -451,7 +429,6 @@ jobs:

# HACK: Despite the name, this environment variable is only used to gate
# tests, so it's applicable to rspack
export TURBOPACK_BUILD=1

node run-tests.js \
--timings \
Expand Down Expand Up @@ -1022,7 +999,6 @@ jobs:
'test-new-tests-start',
'test-new-tests-deploy',
'test-turbopack-production',
'test-turbopack-production-integration',
'test-unit-windows',
'test-dev-windows',
'test-integration-windows',
Expand Down
8 changes: 0 additions & 8 deletions .github/workflows/integration_tests_reusable.yml
Original file line number Diff line number Diff line change
Expand Up @@ -125,14 +125,6 @@ jobs:

export NEXT_TEST_CONTINUE_ON_ERROR=TRUE

# HACK: Despite the name, these environment variables are just used to
# gate tests, so they're applicable to both turbopack and rspack tests
export ${{
inputs.test_type == 'development' &&
'TURBOPACK_DEV=1' ||
'TURBOPACK_BUILD=1'
}}

${{ inputs.run_before_test }}

node run-tests.js \
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ jobs:
name: turbopack-production
test_type: production
run_before_test: |
export IS_TURBOPACK_TEST=1 TURBOPACK_BUILD=1
export IS_TURBOPACK_TEST=1
# Failing tests take longer (due to timeouts and retries). Since we have
# many failing tests, we need smaller groups and longer timeouts, in case
# a group gets stuck with a cluster of failing tests.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,5 @@ jobs:
name: turbopack-development
test_type: development
run_before_test: |
export IS_TURBOPACK_TEST=1 TURBOPACK_DEV=1
export IS_TURBOPACK_TEST=1
secrets: inherit
14 changes: 7 additions & 7 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,25 +21,25 @@
"test-dev-experimental": "cross-env NEXT_TEST_MODE=dev pnpm run with-experimental pnpm testheadless",
"test-dev-rspack": "pnpm run with-rspack pnpm run test-dev",
"test-dev-experimental-rspack": "pnpm run with-rspack pnpm run test-dev-experimental",
"test-dev-turbo": "cross-env NEXT_TEST_MODE=dev IS_TURBOPACK_TEST=1 TURBOPACK_DEV=1 pnpm testheadless",
"test-dev-experimental-turbo": "cross-env NEXT_TEST_MODE=dev IS_TURBOPACK_TEST=1 TURBOPACK_DEV=1 pnpm run with-experimental pnpm testheadless",
"test-dev-turbo": "cross-env NEXT_TEST_MODE=dev IS_TURBOPACK_TEST=1 pnpm testheadless",
"test-dev-experimental-turbo": "cross-env NEXT_TEST_MODE=dev IS_TURBOPACK_TEST=1 pnpm run with-experimental pnpm testheadless",
"test-start": "cross-env NEXT_TEST_MODE=start pnpm testheadless",
"test-start-experimental": "cross-env NEXT_TEST_MODE=start pnpm run with-experimental pnpm testheadless",
"test-start-rspack": "pnpm run with-rspack pnpm run test-start",
"test-start-experimental-rspack": "pnpm run with-rspack pnpm run test-start-experimental",
"test-start-turbo": "cross-env NEXT_TEST_MODE=start IS_TURBOPACK_TEST=1 TURBOPACK_BUILD=1 pnpm testheadless",
"test-start-experimental-turbo": "cross-env NEXT_TEST_MODE=start IS_TURBOPACK_TEST=1 TURBOPACK_BUILD=1 pnpm run with-experimental pnpm testheadless",
"test-start-turbo": "cross-env NEXT_TEST_MODE=start IS_TURBOPACK_TEST=1 pnpm testheadless",
"test-start-experimental-turbo": "cross-env NEXT_TEST_MODE=start IS_TURBOPACK_TEST=1 pnpm run with-experimental pnpm testheadless",
"test-deploy": "cross-env NEXT_TEST_MODE=deploy pnpm testheadless",
"testonly-dev": "cross-env NEXT_TEST_MODE=dev pnpm testonly",
"testonly-dev-rspack": "pnpm run with-rspack pnpm run testonly-dev",
"testonly-dev-turbo": "cross-env NEXT_TEST_MODE=dev IS_TURBOPACK_TEST=1 TURBOPACK_DEV=1 pnpm testonly",
"testonly-dev-turbo": "cross-env NEXT_TEST_MODE=dev IS_TURBOPACK_TEST=1 pnpm testonly",
"testonly-start": "cross-env NEXT_TEST_MODE=start pnpm testonly",
"testonly-start-rspack": "pnpm run with-rspack pnpm run testonly-start",
"testonly-start-turbo": "cross-env NEXT_TEST_MODE=start IS_TURBOPACK_TEST=1 TURBOPACK_BUILD=1 pnpm testonly",
"testonly-start-turbo": "cross-env NEXT_TEST_MODE=start IS_TURBOPACK_TEST=1 pnpm testonly",
"testonly-deploy": "cross-env NEXT_TEST_MODE=deploy pnpm testonly",
"test": "pnpm testheadless",
"test-rspack": "pnpm run with-rspack pnpm run test",
"test-turbo": "cross-env IS_TURBOPACK_TEST=1 TURBOPACK_DEV=1 TURBOPACK_BUILD=1 pnpm testheadless",
"test-turbo": "cross-env IS_TURBOPACK_TEST=1 pnpm testheadless",
"testonly": "jest --runInBand",
"testheadless": "cross-env HEADLESS=true pnpm testonly",
"genstats": "cross-env LOCAL_STATS=true node .github/actions/next-stats-action/src/index.js",
Expand Down
1 change: 0 additions & 1 deletion scripts/test-new-tests.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,6 @@ async function main() {
NEXT_TEST_MODE: testMode,
NEXT_TEST_VERSION: nextTestVersion,
IS_TURBOPACK_TEST: '1',
TURBOPACK_BUILD: testMode === 'start' ? '1' : undefined,
},
})
}
Expand Down
114 changes: 54 additions & 60 deletions test/integration/404-page-app/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,73 +21,67 @@ let appPort
let app

describe('404 Page Support with _app', () => {
;(process.env.TURBOPACK_DEV ? describe.skip : describe)(
'production mode',
() => {
afterAll(() => killApp(app))
describe('production mode', () => {
afterAll(() => killApp(app))

it('should build successfully', async () => {
const { code, stderr, stdout } = await nextBuild(appDir, [], {
stderr: true,
stdout: true,
})
it('should build successfully', async () => {
const { code, stderr, stdout } = await nextBuild(appDir, [], {
stderr: true,
stdout: true,
})

expect(code).toBe(0)
expect(stderr).not.toMatch(gip404Err)
expect(stdout).not.toMatch(gip404Err)
expect(code).toBe(0)
expect(stderr).not.toMatch(gip404Err)
expect(stdout).not.toMatch(gip404Err)

appPort = await findPort()
app = await nextStart(appDir, appPort)
})
appPort = await findPort()
app = await nextStart(appDir, appPort)
})

it('should not output static 404 if _app has getInitialProps', async () => {
const browser = await webdriver(appPort, '/404')
const isAutoExported = await browser.eval('__NEXT_DATA__.autoExport')
expect(isAutoExported).toBeFalsy()
})
it('should not output static 404 if _app has getInitialProps', async () => {
const browser = await webdriver(appPort, '/404')
const isAutoExported = await browser.eval('__NEXT_DATA__.autoExport')
expect(isAutoExported).toBeFalsy()
})

it('specify to use the 404 page still in the routes-manifest', async () => {
const manifest = await fs.readJSON(
join(appDir, '.next/routes-manifest.json')
)
expect(manifest.pages404).toBe(true)
})
it('specify to use the 404 page still in the routes-manifest', async () => {
const manifest = await fs.readJSON(
join(appDir, '.next/routes-manifest.json')
)
expect(manifest.pages404).toBe(true)
})

it('should still use 404 page', async () => {
const res = await fetchViaHTTP(appPort, '/abc')
expect(res.status).toBe(404)
const $ = cheerio.load(await res.text())
expect($('#404-title').text()).toBe('Hi There')
})
}
)
;(process.env.TURBOPACK_BUILD ? describe.skip : describe)(
'development mode',
() => {
let stderr = ''
let stdout = ''
it('should still use 404 page', async () => {
const res = await fetchViaHTTP(appPort, '/abc')
expect(res.status).toBe(404)
const $ = cheerio.load(await res.text())
expect($('#404-title').text()).toBe('Hi There')
})
})
describe('development mode', () => {
let stderr = ''
let stdout = ''

beforeAll(async () => {
appPort = await findPort()
app = await launchApp(appDir, appPort, {
onStderr(msg) {
stderr += msg
},
onStdout(msg) {
stdout += msg
},
})
beforeAll(async () => {
appPort = await findPort()
app = await launchApp(appDir, appPort, {
onStderr(msg) {
stderr += msg
},
onStdout(msg) {
stdout += msg
},
})
afterAll(() => killApp(app))
})
afterAll(() => killApp(app))

it('should not show pages/404 GIP error if _app has GIP', async () => {
const res = await fetchViaHTTP(appPort, '/abc')
expect(res.status).toBe(404)
const $ = cheerio.load(await res.text())
expect($('#404-title').text()).toBe('Hi There')
expect(stderr).not.toMatch(gip404Err)
expect(stdout).not.toMatch(gip404Err)
})
}
)
it('should not show pages/404 GIP error if _app has GIP', async () => {
const res = await fetchViaHTTP(appPort, '/abc')
expect(res.status).toBe(404)
const $ = cheerio.load(await res.text())
expect($('#404-title').text()).toBe('Hi There')
expect(stderr).not.toMatch(gip404Err)
expect(stdout).not.toMatch(gip404Err)
})
})
})
48 changes: 21 additions & 27 deletions test/integration/404-page-custom-error/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,37 +51,31 @@ const runTests = (mode) => {
}

describe('Default 404 Page with custom _error', () => {
;(process.env.TURBOPACK_DEV ? describe.skip : describe)(
'production mode',
() => {
afterAll(() => killApp(app))
describe('production mode', () => {
afterAll(() => killApp(app))

it('should build successfully', async () => {
const { code } = await nextBuild(appDir, [], {
stderr: true,
stdout: true,
})
it('should build successfully', async () => {
const { code } = await nextBuild(appDir, [], {
stderr: true,
stdout: true,
})

expect(code).toBe(0)
expect(code).toBe(0)

appPort = await findPort()
appPort = await findPort()

app = await nextStart(appDir, appPort)
})
app = await nextStart(appDir, appPort)
})

runTests('server')
}
)
;(process.env.TURBOPACK_BUILD ? describe.skip : describe)(
'development mode',
() => {
beforeAll(async () => {
appPort = await findPort()
app = await launchApp(appDir, appPort)
})
afterAll(() => killApp(app))
runTests('server')
})
describe('development mode', () => {
beforeAll(async () => {
appPort = await findPort()
app = await launchApp(appDir, appPort)
})
afterAll(() => killApp(app))

runTests('dev')
}
)
runTests('dev')
})
})
Loading
Loading