diff --git a/package.json b/package.json index 69dea870..72db554e 100644 --- a/package.json +++ b/package.json @@ -7,7 +7,9 @@ "scripts": { "checkFormat": "prettier . --check", "lint": "eslint --cache .", - "test": "node --experimental-vm-modules node_modules/jest/bin/jest.js" + "test:jest": "node --experimental-vm-modules node_modules/jest/bin/jest.js --forceExit", + "test:integration": "node --test 'test/integration/*.test.mjs'", + "test": "npm run test:jest && npm run test:integration" }, "repository": { "type": "git", @@ -34,7 +36,6 @@ }, "jest": { "globalSetup": "./test/js/globalSetup.js", - "globalTeardown": "./test/js/globalTeardown.js", "setupFilesAfterEnv": [ "./test/js/setupTests.js" ], diff --git a/src/plugin.js b/src/plugin.js index 71d2030b..6c8846aa 100644 --- a/src/plugin.js +++ b/src/plugin.js @@ -61,12 +61,20 @@ function getPlugins(opts) { return Array.from(plugins); } +// Counter to ensure unique temp file paths when spawning multiple servers +// with different plugin configurations. +let serverCounter = 0; + // Create a file that will act as a communication mechanism, spawn a parser // server with that filepath as an argument, then wait for the file to be // created that will contain the connection information. export async function spawnServer(opts, killOnExit = true) { const tmpdir = os.tmpdir(); - const filepath = path.join(tmpdir, `prettier-ruby-parser-${process.pid}.txt`); + const suffix = serverCounter++; + const filepath = path.join( + tmpdir, + `prettier-ruby-parser-${process.pid}-${suffix}.txt` + ); const options = { env: Object.assign({}, process.env, { LANG: getLang() }), @@ -76,7 +84,9 @@ export async function spawnServer(opts, killOnExit = true) { if (opts.filepath) { const prettierConfig = await resolveConfigFile(opts.filepath); - options.cwd = path.dirname(prettierConfig); + if (prettierConfig) { + options.cwd = path.dirname(prettierConfig); + } } const server = spawn( @@ -143,18 +153,31 @@ export async function spawnServer(opts, killOnExit = true) { }); } -let connectionOptions; -if (process.env.PRETTIER_RUBY_HOST) { - connectionOptions = JSON.parse(process.env.PRETTIER_RUBY_HOST); +// Map from plugin configuration key to a Promise that resolves to the +// connection options for that server. Using promises as values prevents +// duplicate spawns when concurrent parse calls share the same config. +const servers = new Map(); + +// Derive a stable key from the options that affect server-level behavior +// (i.e. which syntax_tree plugins are loaded). Options like printWidth and +// tabWidth are sent per-request and do not need to be part of the key. +function serverKey(opts) { + return getPlugins(opts).sort().join(","); } // Formats and sends an asynchronous request to the parser server. async function parse(parser, source, opts) { - if (!connectionOptions) { - const spawnedServer = await spawnServer(opts); - connectionOptions = spawnedServer.connectionOptions; + const key = serverKey(opts); + + if (!servers.has(key)) { + servers.set( + key, + spawnServer(opts).then((s) => s.connectionOptions) + ); } + const connOpts = await servers.get(key); + return new Promise((resolve, reject) => { const socket = new net.Socket(); let chunks = ""; @@ -176,13 +199,13 @@ async function parse(parser, source, opts) { error.loc = response.loc; } - reject(error); + return reject(error); } resolve(response); }); - socket.connect(connectionOptions, () => { + socket.connect(connOpts, () => { socket.end( JSON.stringify({ parser, diff --git a/test/integration/server_isolation.test.mjs b/test/integration/server_isolation.test.mjs new file mode 100644 index 00000000..2f3df1b8 --- /dev/null +++ b/test/integration/server_isolation.test.mjs @@ -0,0 +1,146 @@ +// Integration tests for server isolation. +// +// These tests verify that formatting Ruby files produces consistent output +// regardless of whether markdown files with embedded Ruby are also processed. +// +// The bug: the plugin spawned a single Ruby server whose syntax_tree plugins +// were determined by whichever file triggered the first parse() call. When a +// markdown file with a ```ruby fenced block was processed first, Prettier +// passed the markdown parser's resolved options (trailingComma: "all") instead +// of the ruby plugin's defaults (trailingComma: "none"). This loaded the +// trailing_comma plugin into the shared server, adding trailing commas to +// every subsequent .rb file. +// +// These tests run prettier as a subprocess so each invocation gets a fresh +// module and spawns its own server processes. + +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { execFileSync } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import url from "node:url"; + +const ROOT = path.resolve(url.fileURLToPath(import.meta.url), "../../.."); +const PRETTIER = path.join(ROOT, "node_modules/.bin/prettier"); +const PLUGIN = path.join(ROOT, "src/plugin.js"); + +// A Ruby snippet whose formatting changes when the trailing_comma plugin is +// loaded. The argument names are long enough that they always wrap to multiple +// lines at printWidth 80 (the ruby plugin default), which means the +// trailing_comma plugin will append a comma after the last argument. +const RUBY_SOURCE = `\ +class Example + def call + some_long_method_name( + first_really_long_argument, + second_really_long_argument + ) + end +end +`; + +// Markdown containing a ```ruby fenced block with hash-rocket syntax. +// This is the trigger: when prettier formats this file, the embedded ruby +// parser may receive trailingComma: "all" from the markdown parser context. +const MARKDOWN_SOURCE = `\ +# Example + +\`\`\`ruby +SORT_KEYS = { + RelativeTime => [:created_at], + Status => [:status] +} +\`\`\` +`; + +// Minimal prettierrc that matches the ruby plugin's own defaults. +const PRETTIERRC = JSON.stringify({ plugins: [PLUGIN] }); + +function makeTmpDir() { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "prettier-ruby-test-")); + // Write a prettierrc so prettier resolves config consistently. + fs.writeFileSync(path.join(dir, ".prettierrc.json"), PRETTIERRC); + return dir; +} + +function runPrettierStatus(args, opts = {}) { + try { + const stdout = execFileSync( + process.execPath, + [PRETTIER, "--plugin", PLUGIN, ...args], + { + cwd: opts.cwd || ROOT, + input: opts.input, + encoding: "utf-8", + timeout: 30_000, + stdio: ["pipe", "pipe", "pipe"] + } + ); + return { status: 0, stdout }; + } catch (err) { + return { status: err.status, stdout: err.stdout, stderr: err.stderr }; + } +} + +describe("server isolation", () => { + it("formatting .rb alone produces same output as with .md present", () => { + // Format ruby in isolation. + const isolatedDir = makeTmpDir(); + try { + fs.writeFileSync(path.join(isolatedDir, "sample.rb"), RUBY_SOURCE); + runPrettierStatus(["--write", isolatedDir]); + const isolated = fs.readFileSync( + path.join(isolatedDir, "sample.rb"), + "utf-8" + ); + + // Format the same ruby with a markdown file present. + const mixedDir = makeTmpDir(); + try { + fs.writeFileSync(path.join(mixedDir, "a_docs.md"), MARKDOWN_SOURCE); + fs.writeFileSync(path.join(mixedDir, "b_sample.rb"), RUBY_SOURCE); + runPrettierStatus(["--write", mixedDir]); + const withMarkdown = fs.readFileSync( + path.join(mixedDir, "b_sample.rb"), + "utf-8" + ); + + assert.equal( + withMarkdown, + isolated, + "Formatting .rb with .md present produced different output " + + "than formatting .rb alone.\n" + + `Isolated:\n${isolated}\nWith markdown:\n${withMarkdown}` + ); + } finally { + fs.rmSync(mixedDir, { recursive: true, force: true }); + } + } finally { + fs.rmSync(isolatedDir, { recursive: true, force: true }); + } + }); + + it("does not add trailing commas from markdown context", () => { + const dir = makeTmpDir(); + try { + fs.writeFileSync(path.join(dir, "a_docs.md"), MARKDOWN_SOURCE); + fs.writeFileSync(path.join(dir, "b_sample.rb"), RUBY_SOURCE); + + runPrettierStatus(["--write", dir]); + const result = fs.readFileSync(path.join(dir, "b_sample.rb"), "utf-8"); + + // The trailing_comma plugin would turn "second_really_long_argument\n" + // into "second_really_long_argument,\n". Verify this didn't happen. + assert.ok( + !result.includes("second_really_long_argument,"), + "Trailing comma was added to .rb file — " + + "markdown context leaked into ruby server.\n" + + `Formatted output:\n${result}` + ); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/test/js/globalSetup.js b/test/js/globalSetup.js index 72c9be34..75bf4239 100644 --- a/test/js/globalSetup.js +++ b/test/js/globalSetup.js @@ -1,9 +1,5 @@ import { spawnSync } from "child_process"; -import { spawnServer } from "../../src/plugin.js"; -// This is somewhat similar to the spawnServer function in parseSync but -// slightly different in that it logs its information into environment variables -// so that it can be reused across the test suite. async function globalSetup() { // Set a RUBY_VERSION environment variable because certain tests will only run // for certain versions of Ruby. @@ -11,20 +7,6 @@ async function globalSetup() { process.env.RUBY_VERSION = spawnSync("ruby", args) .stdout.toString("utf-8") .trim(); - - const { serverPID, connectionFilepath, connectionOptions } = - await spawnServer( - { - rubyPlugins: "", - rubySingleQuote: false, - trailingComma: "none" - }, - false - ); - - process.env.PRETTIER_RUBY_PID = serverPID; - process.env.PRETTIER_RUBY_FILE = connectionFilepath; - process.env.PRETTIER_RUBY_HOST = JSON.stringify(connectionOptions); } export default globalSetup; diff --git a/test/js/globalTeardown.js b/test/js/globalTeardown.js deleted file mode 100644 index c51948b9..00000000 --- a/test/js/globalTeardown.js +++ /dev/null @@ -1,25 +0,0 @@ -import fs from "fs"; - -// If a parse server was successfully spawned, then its process ID will be in -// the PRETTIER_RUBY_PID environment variable. At the end of the test suite we -// should send a kill signal to it. -function globalTeardown() { - const serverPID = process.env.PRETTIER_RUBY_PID; - const connectionFilepath = process.env.PRETTIER_RUBY_FILE; - - if (serverPID) { - try { - const pid = process.platform === "win32" ? serverPID : -serverPID; - process.kill(pid, "SIGINT"); - } catch (error) { - console.error("Failed to kill the parser process in globalTeardown."); - throw error; - } - } - - if (fs.existsSync(connectionFilepath)) { - fs.unlinkSync(connectionFilepath); - } -} - -export default globalTeardown;