Repository navigation
test: remove the Windows skips that are not needed #46479
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
32448a5
1df2011
b92b931
42149e7
2688246
be9cbee
14c7bbd
50a726c
a06feb7
4b98735
53cb671
97d64a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -521,20 +521,17 @@ describe.concurrent("bun fmt", () => { | |
| expect(result.exitCode).toBe(0); | ||
| }); | ||
|
|
||
| // A file name can have at most 255 bytes. On Windows the path would also be too long. | ||
| test.skipIf(isWindows)( | ||
| "can have the longest possible name, and is still replaced rather than written in place", | ||
| async () => { | ||
| const name = `${Buffer.alloc(252, "a")}.ts`; | ||
| using dir = tempDir("bun-format", { [name]: ugly }); | ||
| const before = statSync(join(String(dir), name)).ino; | ||
| const { exitCode } = await write(String(dir), []); | ||
| expect(read(String(dir), name)).toBe(formatted); | ||
| expect(statSync(join(String(dir), name)).ino).not.toBe(before); | ||
| expect(readdirSync(String(dir))).toEqual([name]); | ||
| expect(exitCode).toBe(0); | ||
| }, | ||
| ); | ||
| // A file name can have at most 255 bytes. | ||
| test("can have the longest possible name, and is still replaced rather than written in place", async () => { | ||
| const name = `${Buffer.alloc(252, "a")}.ts`; | ||
| using dir = tempDir("bun-format", { [name]: ugly }); | ||
| const before = statSync(join(String(dir), name)).ino; | ||
| const { exitCode } = await write(String(dir), []); | ||
| expect(read(String(dir), name)).toBe(formatted); | ||
| expect(statSync(join(String(dir), name)).ino).not.toBe(before); | ||
| expect(readdirSync(String(dir))).toEqual([name]); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| // For example in a container with a user's project mounted into it. | ||
| test.skipIf(!isRoot)("keeps its owner if root formats it", async () => { | ||
|
|
@@ -552,7 +549,7 @@ describe.concurrent("bun fmt", () => { | |
| }); | ||
|
|
||
| // Prettier doesn't need write access to the directory. | ||
| test.skipIf(isWindows || isRoot)("can be in a read-only directory", async () => { | ||
| test.skipIf(isRoot)("can be in a read-only directory", async () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🔎 Supported by static analysis
Make the Windows fixture deny directory writes. On Windows,
|
||
| using dir = tempDir("bun-format", { "src/a.js": ugly }); | ||
| chmodSync(join(String(dir), "src"), 0o555); | ||
| try { | ||
|
|
@@ -5731,15 +5728,12 @@ exports.format = async text => "by Prettier"; | |
| expect(result.exitCode).toBe(2); | ||
| }); | ||
|
|
||
| test.skipIf(isWindows)( | ||
| "/dev/null works as an ignore file without patterns and as a configuration without options", | ||
| async () => { | ||
| const files = { ".prettierignore": "a.js\n", ".prettierrc": `{ "semi": false }\n`, "a.js": formatted }; | ||
| expect(await different(files, ["."])).toEqual([]); | ||
| expect(await different(files, ["--ignore-path", "/dev/null", "."])).toEqual(["a.js"]); | ||
| expect(await different(files, ["--ignore-path", "/dev/null", "--config", "/dev/null", "."])).toEqual([]); | ||
| }, | ||
| ); | ||
| test("/dev/null works as an ignore file without patterns and as a configuration without options", async () => { | ||
| const files = { ".prettierignore": "a.js\n", ".prettierrc": `{ "semi": false }\n`, "a.js": formatted }; | ||
| expect(await different(files, ["."])).toEqual([]); | ||
| expect(await different(files, ["--ignore-path", "/dev/null", "."])).toEqual(["a.js"]); | ||
| expect(await different(files, ["--ignore-path", "/dev/null", "--config", "/dev/null", "."])).toEqual([]); | ||
| }); | ||
|
|
||
| // This means "never break a line". Prettier accepts every integer from 0 up, and Infinity. | ||
| test.each([ | ||
|
|
@@ -6162,7 +6156,7 @@ exports.format = async text => "by Prettier"; | |
| expect(result.exitCode).toBe(0); | ||
| }); | ||
|
|
||
| test.skipIf(isWindows)("a named pipe whose name ends in .js is skipped, like in Prettier", async () => { | ||
| test("a named pipe whose name ends in .js is skipped, like in Prettier", async () => { | ||
| const before = (dir: string) => expect(Bun.spawnSync(["mkfifo", join(dir, "pipe.js")]).exitCode).toBe(0); | ||
| expect(await different({ "a.js": ugly }, [], { before })).toEqual(["a.js"]); | ||
| }); | ||
|
|
@@ -6634,7 +6628,7 @@ exports.format = async text => "by Prettier"; | |
| // A case's guard used to be printed without a stack check, so the stack overflowed at depths between the printer's | ||
| // limit and the parser's. The threads that format files have stack to spare, so this uses the main thread with half | ||
| // of its usual stack. | ||
| test.skipIf(isWindows || isDebug || isASAN).concurrent.each([4_500, 4_612, 4_728, 4_846, 4_967, 5_091])( | ||
| test.skipIf(isDebug || isASAN).concurrent.each([4_500, 4_612, 4_728, 4_846, 4_967, 5_091])( | ||
| "Flow's match with %d nested guards, on a 4 MB stack", | ||
| async depth => { | ||
| using dir = tempDir("bun-format-small-stack", {}); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Windows developers running this file locally now get three "dev server" tests that fail or hang, where the base marked them todo there, and no CI lane can have verified them. test/cli/create/create-jsx.test.ts:129 (and :196, :262) keeps todoIf(isCI), so on every Buildkite lane these tests are skipped as todo and never ran; the PR's own pass criterion cannot have applied. The sibling "build" test at :164 starts with the same
bun create ./index.jsxplus getServerUrl and stays todoIf(isWindows), which says that flow still fails on Windows. Fix: keepisCI || isWindowson the three dev-server tests until a Windows run exists that shows them passing.Why this was flagged
The trigger is a developer running
bun bd test test/cli/create/create-jsx.test.tson Windows, where isCI is false. On the base branch test.todoIf(isCI || isWindows) at :129, :196 and :262 made the three dev-server tests todo there. After the change they run: they spawnbun create ./index.jsx(:131), wait for a server URL, fetch the page and compare two snapshots (:144, :158). On CI the tests are still todo because isCI is true, so neither Windows lane ever executed them; the PR's stated rule that a skip is removed only when the test passed on both Windows lanes was not satisfiable for these. The same file keeps the "build" test at :164 as todoIf(isWindows), and that test begins with the identicalbun create ./index.jsxand getServerUrl step (:166-173), which is the author's own evidence that the create flow is not known to work on Windows. Remedy: restore the Windows term on the three dev-server tests, or run them on a Windows lane before removing it.Verification: normal — triggered when a developer runs this file locally on Windows (isCI false), where the three "dev server" tests now execute instead of being todo.
isCIisprocess.env.CI !== undefined(test/harness.ts:42), so on every Buildkite lane these tests remain todo and were never executed. Fix is to restoreisWindowson :129, :196, :262 as was done at :301.