From 568e170a91c9a0f3ac2b2b02f358662a530d9166 Mon Sep 17 00:00:00 2001 From: Marcin Biernacik Date: Mon, 28 Sep 2026 18:10:35 +0000 Subject: [PATCH 01/14] Support Dataform CLI on Windows and add automated CI - Enforce canonical forward slash '/' separators across all platforms via Path.toPosixPath in core/path.ts - Use Path.toPosixPath in core/session.ts, core/utils.ts, and core/main.ts instead of inline regex replacements - Support case-insensitive Windows drive letters in relativePath and caller file detection - Allow file: tarball dependencies in workflow_settings.yaml in core/workflow_settings.ts - Remove unsupported stdio pipe and SIGKILL in cli/api/commands/base_worker.ts - Add maxRetries and recursive cleanup to cli/api/commands/compile.ts to prevent Windows file locking issues - Add CRLF normalization helper to cli/commands/format_command.ts - Add scripts/smoke_test_windows.js testing all primary CLI flows under PowerShell and cmd.exe - Add .github/workflows/test.yaml with windows-latest runner and status checks - Update contributing.md with Windows prerequisites and smoke testing instructions --- .github/workflows/test.yaml | 77 +++++ cli/api/commands/base_worker.ts | 4 +- cli/api/commands/compile.ts | 2 +- cli/commands/format_command.ts | 8 +- contributing.md | 51 +++ core/BUILD | 1 + core/main.ts | 5 + core/path.ts | 46 +-- core/path_test.ts | 77 +++++ core/session.ts | 11 +- core/utils.ts | 13 +- core/workflow_settings.ts | 6 +- scripts/smoke_test_windows.js | 543 ++++++++++++++++++++++++++++++++ 13 files changed, 808 insertions(+), 36 deletions(-) create mode 100644 .github/workflows/test.yaml create mode 100644 core/path_test.ts create mode 100755 scripts/smoke_test_windows.js diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml new file mode 100644 index 000000000..e8ff5ea30 --- /dev/null +++ b/.github/workflows/test.yaml @@ -0,0 +1,77 @@ +name: Test + +on: + pull_request: + push: + branches: [main, windows-support] + workflow_dispatch: + +jobs: + test-linux: + name: Build and test (Linux) + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Run unit tests + uses: gcr.io/cloud-builders/bazel@sha256:aa6c5ffd0d6d37960715d9c3392ba885e6484322c81c3604a90ccfb15c07c110 + entrypoint: bash + with: + args: ./scripts/run_tests + + - name: Build package tarballs + uses: gcr.io/cloud-builders/bazel@sha256:aa6c5ffd0d6d37960715d9c3392ba885e6484322c81c3604a90ccfb15c07c110 + entrypoint: bash + with: + args: bazel build //packages/@dataform/cli:package_tar //packages/@dataform/core:package_tar + + - name: Upload CLI tarball + uses: actions/upload-artifact@v4 + with: + name: dataform-cli-tarball + path: bazel-bin/packages/@dataform/cli/package.tar.gz + + - name: Upload Core tarball + uses: actions/upload-artifact@v4 + with: + name: dataform-core-tarball + path: bazel-bin/packages/@dataform/core/package.tar.gz + + test-windows: + name: CLI smoke & unit tests (Windows) + needs: test-linux + runs-on: windows-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version: 20 + + - name: Download CLI tarball + uses: actions/download-artifact@v4 + with: + name: dataform-cli-tarball + path: dist/cli + + - name: Download Core tarball + uses: actions/download-artifact@v4 + with: + name: dataform-core-tarball + path: dist/core + + - name: Install @dataform/cli globally from tarball + run: npm install -g dist/cli/package.tar.gz + + - name: Run Windows smoke tests in PowerShell + shell: powershell + run: | + node scripts/smoke_test_windows.js --shell powershell --core-tarball dist/core/package.tar.gz + + - name: Run Windows smoke tests in cmd.exe + shell: cmd + run: | + node scripts\smoke_test_windows.js --shell cmd --core-tarball dist\core\package.tar.gz diff --git a/cli/api/commands/base_worker.ts b/cli/api/commands/base_worker.ts index 941fdae9f..436d9c015 100644 --- a/cli/api/commands/base_worker.ts +++ b/cli/api/commands/base_worker.ts @@ -19,7 +19,7 @@ export abstract class BaseWorker { ): Promise { const forkScript = this.resolveScript(); const child = fork(forkScript, [], { - stdio: [0, 1, 2, "ipc", "pipe"], + stdio: [0, 1, 2, "ipc"], }); return new Promise((resolve, reject) => { @@ -32,7 +32,7 @@ export abstract class BaseWorker { } completed = true; clearTimeout(timeout); - child.kill("SIGKILL"); + child.kill(); fn(); }; diff --git a/cli/api/commands/compile.ts b/cli/api/commands/compile.ts index 5cace57af..993e9ac88 100644 --- a/cli/api/commands/compile.ts +++ b/cli/api/commands/compile.ts @@ -96,7 +96,7 @@ export async function compile( compiledGraph = dataform.CompiledGraph.create(decodedResult.compile.compiledGraph); if (workflowSettingsDataformCoreVersion) { - fs.rmSync(temporaryProjectPath, { recursive: true }); + fs.rmSync(temporaryProjectPath, { recursive: true, force: true, maxRetries: 3 }); } return compiledGraph; diff --git a/cli/commands/format_command.ts b/cli/commands/format_command.ts index 2f6fda427..8d822f982 100644 --- a/cli/commands/format_command.ts +++ b/cli/commands/format_command.ts @@ -12,8 +12,11 @@ import { } from "df/cli/common_options"; import { printError, printFormatFilesResult, printSuccess } from "df/cli/console"; import { ICommand, INamedOption } from "df/cli/yargswrapper"; +import { toPosixPath } from "df/core/path"; import { formatFile } from "df/sqlx/format"; +const normalizeLineEndings = (text: string) => text.replace(/\r\n/g, "\n"); + export interface IFormatArgs extends IProjectDirArgs, IActionsArgs { ignoreJsFiles: boolean; check: boolean; @@ -50,7 +53,7 @@ export const formatCommand: ICommand = { actions = argv.actions; } const filenames = actions - .map((action: string) => glob.sync(action, { cwd: argv.projectDir })) + .map((action: string) => glob.sync(toPosixPath(action), { cwd: argv.projectDir })) .flat(); const isCheckMode = argv.check; @@ -70,7 +73,8 @@ export const formatCommand: ICommand = { }); return { filename, - needsFormatting: fileContent !== formattedContent, + needsFormatting: + normalizeLineEndings(fileContent) !== normalizeLineEndings(formattedContent), }; } else { // Normal formatting mode diff --git a/contributing.md b/contributing.md index 96047b8a6..b662f0b2b 100644 --- a/contributing.md +++ b/contributing.md @@ -115,6 +115,57 @@ pnpm will use the registry specified in ~/.npmrc by default. When building in th Additionally, installed NPM dependencies need to be added to the `deps` of `ts_library` rules by prefixing them with `//:node_modules/...`. +## Windows Support + +The Dataform CLI is supported on Windows 10 and 11 in both **Command Prompt (`cmd.exe`)** and **PowerShell**. + +### Prerequisites & Setup +- **Node.js**: Node 18+ or Node 20+ LTS is required. +- **Git line endings**: Configure git to handle CRLF/LF cleanly: + ```bash + git config --global core.autocrlf true + ``` + Dataform format commands (`dataform format` / `dataform format --check`) are CRLF-aware and normalize line endings before checking. +- **Symlinks & Long Paths**: If using Bazel or deep `node_modules` hierarchies on Windows, enable [Windows Developer Mode](https://learn.microsoft.com/en-us/windows/apps/get-started/enable-your-device-for-development) or enable `core.longpaths` in Git: + ```bash + git config --global core.longpaths true + ``` + +### Development & Path Conventions +- **Canonical Forward Slashes**: Dataform's internal graph contracts, table names, and proto file paths require canonical POSIX forward slashes (`/`). Even on Windows, definitions must resolve as `definitions/my_table.sqlx` rather than with backslashes (`\`). +- **Cross-Platform Path Helpers**: Use the `Path` utility in `core/path.ts` (`Path.join`, `Path.normalize`, `Path.relativePath`, `Path.dirName`, `Path.basename`). It handles mixed separators and Windows drive letters (e.g. `C:\` vs `c:/`) automatically. + +### Running Windows Smoke Tests +You can run the automated Windows smoke test suite locally on Windows or in CI: + +- **In PowerShell**: + ```powershell + npm install -g @dataform/cli + node scripts/smoke_test_windows.js --shell powershell + ``` + +- **In Command Prompt (`cmd.exe`)**: + ```cmd + npm install -g @dataform/cli + node scripts\smoke_test_windows.js --shell cmd + ``` + +- **Testing local package tarballs**: + ```powershell + node scripts/smoke_test_windows.js --core-tarball path/to/@dataform/core/package.tar.gz + ``` + +### Windows CI on GitHub Actions +All pull requests run automated CI on `windows-latest` via `.github/workflows/test.yaml`. +To make the Windows test job a required status check on your repository: +1. Go to **Settings** > **Branches** > **Branch protection rules**. +2. Edit the rule for `main`. +3. Check **Require status checks to pass before merging**. +4. Search for and select: + - `Build and test (Linux)` + - `CLI smoke & unit tests (Windows)` + + ## The Contribution Process 1. Decide on what you'd like to contribute. The majority of open-source contributions come from: diff --git a/core/BUILD b/core/BUILD index 10c88a9a1..b8e1d94cf 100644 --- a/core/BUILD +++ b/core/BUILD @@ -94,6 +94,7 @@ ts_test_suite( "main_session_test.ts", "main_sqlx_chars_test.ts", "main_workflow_settings_test.ts", + "path_test.ts", "utils_test.ts", "workflow_settings_test.ts", ], diff --git a/core/main.ts b/core/main.ts index d218d9c7c..7cf49bd26 100644 --- a/core/main.ts +++ b/core/main.ts @@ -38,6 +38,11 @@ export function main(coreExecutionRequest: Uint8Array | string): Uint8Array | st request = dataform.CoreExecutionRequest.decode(coreExecutionRequest); } const compileRequest = request.compile; + if (compileRequest?.compileConfig?.filePaths) { + compileRequest.compileConfig.filePaths = compileRequest.compileConfig.filePaths.map( + Path.toPosixPath, + ); + } // Allow extensions to populate settings by themselves. const failIfMissing = !compileRequest?.compileConfig?.extension?.compilationMode; diff --git a/core/path.ts b/core/path.ts index 51f0e74f2..9a35450da 100644 --- a/core/path.ts +++ b/core/path.ts @@ -1,37 +1,43 @@ -export const separator = (() => { - if (typeof process !== "undefined") { - return process.platform === "win32" ? "\\" : "/"; - } - return "/"; -})(); +export const separator = "/"; + +export function toPosixPath(path: string): string { + return path ? path.replace(/\\+/g, "/") : path; +} export function relativePath(fullPath: string, base: string) { + const normalizedFull = toPosixPath(fullPath); if (base.length === 0) { - return fullPath; + return normalizedFull; } - const stripped = fullPath.substr(base.length); - if (stripped.startsWith(separator)) { - return stripped.substr(1); - } else { - return stripped; + let normalizedBase = toPosixPath(base); + if (!normalizedBase.endsWith("/")) { + normalizedBase += "/"; + } + if (normalizedFull.toLowerCase().startsWith(normalizedBase.toLowerCase())) { + return normalizedFull.slice(normalizedBase.length); } + return normalizedFull; } export function filename(path: string) { - return path.split(separator).slice(-1)[0]; + return toPosixPath(path).split("/").slice(-1)[0]; } export function basename(path: string) { const f = filename(path); - return f.substring(0, f.lastIndexOf(".")); + const dotIndex = f.lastIndexOf("."); + return dotIndex === -1 ? f : f.substring(0, dotIndex); } export function dirName(fullPath: string) { - return fullPath.slice(0, fullPath.lastIndexOf(separator)); + const normalized = toPosixPath(fullPath); + const lastSlash = normalized.lastIndexOf("/"); + return lastSlash === -1 ? "" : normalized.slice(0, lastSlash); } export function join(...paths: string[]) { return paths + .map(toPosixPath) .map((path) => { if (path.startsWith(separator)) { path = path.slice(1); @@ -41,6 +47,7 @@ export function join(...paths: string[]) { } return path; }) + .filter((path) => path.length > 0) .join(separator); } @@ -49,13 +56,16 @@ export function escapedBasename(path: string) { } export function fileExtension(fullPath: string) { - return fullPath.split(".").slice(-1)[0]; + const f = filename(fullPath); + const dotIndex = f.lastIndexOf("."); + return dotIndex === -1 ? "" : f.slice(dotIndex + 1); } export function normalize(path: string) { + const normalized = toPosixPath(path); const parts = []; let dotDotCount = 0; - for (const part of path.split("/").filter((p) => !!p && p !== ".")) { + for (const part of normalized.split("/").filter((p) => !!p && p !== ".")) { if (part === "..") { if (parts.length === 0) { dotDotCount++; @@ -66,7 +76,7 @@ export function normalize(path: string) { parts.push(part); } } - if (path.startsWith("/")) { + if (normalized.startsWith("/")) { if (parts.length === 0) { return "/"; } diff --git a/core/path_test.ts b/core/path_test.ts new file mode 100644 index 000000000..dd837e3fe --- /dev/null +++ b/core/path_test.ts @@ -0,0 +1,77 @@ +import { expect } from "chai"; + +import * as Path from "df/core/path"; +import { suite, test } from "df/testing"; + +suite("path utils", () => { + test("separator is always forward slash", () => { + expect(Path.separator).equals("/"); + }); + + test("toPosixPath converts Windows backslashes to forward slashes", () => { + expect(Path.toPosixPath("definitions\\table.sqlx")).equals("definitions/table.sqlx"); + expect(Path.toPosixPath("C:\\Users\\runner\\project\\file.sqlx")).equals( + "C:/Users/runner/project/file.sqlx", + ); + expect(Path.toPosixPath("definitions/table.sqlx")).equals("definitions/table.sqlx"); + expect(Path.toPosixPath("")).equals(""); + }); + + test("filename extracts file name with POSIX and Windows slashes", () => { + expect(Path.filename("definitions/table.sqlx")).equals("table.sqlx"); + expect(Path.filename("definitions\\table.sqlx")).equals("table.sqlx"); + expect(Path.filename("C:\\Users\\runner\\project\\definitions\\table.sqlx")).equals( + "table.sqlx", + ); + expect(Path.filename("table.sqlx")).equals("table.sqlx"); + }); + + test("basename extracts base name without extension", () => { + expect(Path.basename("definitions/table.sqlx")).equals("table"); + expect(Path.basename("definitions\\table.sqlx")).equals("table"); + expect(Path.basename("C:\\Users\\runner\\project\\definitions\\table.sqlx")).equals("table"); + expect(Path.basename("no_ext")).equals("no_ext"); + expect(Path.basename("multi.dot.name.sqlx")).equals("multi.dot.name"); + }); + + test("dirName extracts directory path with POSIX and Windows slashes", () => { + expect(Path.dirName("definitions/table.sqlx")).equals("definitions"); + expect(Path.dirName("definitions\\table.sqlx")).equals("definitions"); + expect(Path.dirName("C:\\project\\definitions\\table.sqlx")).equals("C:/project/definitions"); + expect(Path.dirName("table.sqlx")).equals(""); + }); + + test("relativePath normalizes to forward slashes across platforms", () => { + expect(Path.relativePath("/path/to/project/definitions/file.sqlx", "/path/to/project")).equals( + "definitions/file.sqlx", + ); + expect(Path.relativePath("C:\\project\\definitions\\file.sqlx", "C:\\project")).equals( + "definitions/file.sqlx", + ); + expect(Path.relativePath("c:\\project\\definitions\\file.sqlx", "C:\\project")).equals( + "definitions/file.sqlx", + ); + expect(Path.relativePath("C:/project/definitions/file.sqlx", "C:\\project")).equals( + "definitions/file.sqlx", + ); + expect(Path.relativePath("file.sqlx", "")).equals("file.sqlx"); + }); + + test("join combines paths with forward slashes", () => { + expect(Path.join("a", "b", "c")).equals("a/b/c"); + expect(Path.join("a\\b", "c\\d")).equals("a/b/c/d"); + expect(Path.join("/a/", "/b/")).equals("a/b"); + }); + + test("fileExtension extracts extension", () => { + expect(Path.fileExtension("definitions/file.sqlx")).equals("sqlx"); + expect(Path.fileExtension("definitions\\file.sqlx")).equals("sqlx"); + expect(Path.fileExtension("no_extension")).equals(""); + }); + + test("normalize handles both slash types and dot navigation", () => { + expect(Path.normalize("a/b/../c")).equals("a/c"); + expect(Path.normalize("a\\b\\..\\c")).equals("a/c"); + expect(Path.normalize("/a/b/../../c")).equals("/c"); + }); +}); diff --git a/core/session.ts b/core/session.ts index d0ac460f5..393a94574 100644 --- a/core/session.ts +++ b/core/session.ts @@ -104,13 +104,14 @@ export class Session { public getContents(filePath: string): string { const callerFile = utils.getCallerFile(this.rootDir); const callerDir = Path.dirName(callerFile); + const normalizedRootDir = Path.toPosixPath(this.rootDir); const resolvedPath = Path.join(callerDir, filePath); - const absolutePath = Path.separator + Path.normalize(Path.join(this.rootDir, resolvedPath)); - const rootDir = this.rootDir.endsWith(Path.separator) - ? this.rootDir - : this.rootDir + Path.separator; + const absolutePath = Path.normalize(Path.join(normalizedRootDir, resolvedPath)); + const rootDirWithSlash = normalizedRootDir.endsWith("/") + ? normalizedRootDir + : normalizedRootDir + "/"; - if (!absolutePath.startsWith(rootDir)) { + if (!absolutePath.toLowerCase().startsWith(rootDirWithSlash.toLowerCase())) { throw new Error(`Cannot read "${filePath}": path resolves outside the project directory.`); } diff --git a/core/utils.ts b/core/utils.ts index 59bf628a8..d03ce8479 100644 --- a/core/utils.ts +++ b/core/utils.ts @@ -46,12 +46,14 @@ export function matchPatterns(patterns: string[], values: string[]) { export function getCallerFile(rootDir: string) { let lastfile: string; const stack = getCurrentStack(); + const normalizedRootDir = Path.toPosixPath(rootDir); while (stack.length) { - const nextLastfile = stack.shift().getFileName(); - if (!nextLastfile) { + const rawNextLastfile = stack.shift().getFileName(); + if (!rawNextLastfile) { continue; } - if (!nextLastfile.includes(rootDir)) { + const nextLastfile = Path.toPosixPath(rawNextLastfile); + if (!nextLastfile.toLowerCase().includes(normalizedRootDir.toLowerCase())) { continue; } if (nextLastfile.includes("node_modules")) { @@ -60,10 +62,7 @@ export function getCallerFile(rootDir: string) { // If it's in the root directory we'll take it, but keep searching // for a better match. lastfile = nextLastfile; - if (!( - nextLastfile.includes(`definitions${Path.separator}`) || - nextLastfile.includes(`models${Path.separator}`) - )) { + if (!(nextLastfile.includes("definitions/") || nextLastfile.includes("models/"))) { continue; } break; diff --git a/core/workflow_settings.ts b/core/workflow_settings.ts index 033b5ef4d..f52d471e1 100644 --- a/core/workflow_settings.ts +++ b/core/workflow_settings.ts @@ -70,7 +70,11 @@ function verifyWorkflowSettingsAsJson(workflowSettingsAsJson: object): dataform. } // The caller of Dataform Core should ensure that the correct version is installed. - if (!!workflowSettings.dataformCoreVersion && workflowSettings.dataformCoreVersion !== version) { + if ( + !!workflowSettings.dataformCoreVersion && + workflowSettings.dataformCoreVersion !== version && + !workflowSettings.dataformCoreVersion.startsWith("file:") + ) { throw Error( `Version mismatch: workflow settings specifies version ${workflowSettings.dataformCoreVersion}` + `, but ${version} was found`, diff --git a/scripts/smoke_test_windows.js b/scripts/smoke_test_windows.js new file mode 100755 index 000000000..4b41af67d --- /dev/null +++ b/scripts/smoke_test_windows.js @@ -0,0 +1,543 @@ +#!/usr/bin/env node +/** + * Automated smoke and unit test suite for Dataform CLI on Windows. + * + * Exercises all primary CLI flows: + * 1. Version / Help check + * 2. dataform init + * 3. dataform install (workflow_settings.yaml and package.json flows) + * 4. dataform compile (default, --json, JiT action, --watch) + * 5. dataform run --dry-run + * 6. dataform test + * 7. dataform format (with --check and CRLF handling) + * 8. dataform init-creds (--help) + * + * Can run under cmd.exe, PowerShell, or bash. + */ + +const { spawn, spawnSync } = require("child_process"); +const fs = require("fs"); +const path = require("path"); +const os = require("os"); +const assert = require("assert"); + +// Parse CLI flags +const args = process.argv.slice(2); +function getArg(flag, defaultValue = null) { + const index = args.indexOf(flag); + if (index !== -1 && index + 1 < args.length) { + return args[index + 1]; + } + return defaultValue; +} + +const targetShell = getArg("--shell", "auto"); // "powershell", "cmd", or "auto" +const customBin = getArg("--dataform-bin", process.env.DATAFORM_BIN || null); +const coreTarball = getArg("--core-tarball", null); + +console.log("=== Dataform Windows Smoke & Unit Test Runner ==="); +console.log(`OS: ${os.platform()} (${os.release()})`); +console.log(`Node: ${process.version}`); +console.log(`Shell target: ${targetShell}`); +if (customBin) console.log(`Custom dataform binary: ${customBin}`); +if (coreTarball) console.log(`Core tarball: ${coreTarball}`); +console.log("=================================================\n"); + +let passed = 0; +let failed = 0; +let skipped = 0; + +function runTest(name, fn) { + process.stdout.write(`TEST: ${name} ... `); + try { + const result = fn(); + if (result && typeof result.then === "function") { + return result + .then(() => { + console.log("PASSED"); + passed++; + }) + .catch((err) => { + console.log("FAILED"); + console.error(` Error: ${err.message || err}`); + if (err.stack) console.error(err.stack.split("\n").slice(0, 5).join("\n")); + failed++; + }); + } else { + console.log("PASSED"); + passed++; + return Promise.resolve(); + } + } catch (err) { + console.log("FAILED"); + console.error(` Error: ${err.message || err}`); + if (err.stack) console.error(err.stack.split("\n").slice(0, 5).join("\n")); + failed++; + return Promise.resolve(); + } +} + +function resolveDataformCmd() { + if (customBin) { + return customBin; + } + if (os.platform() === "win32") { + // Look for dataform.cmd in PATH + const pathDirs = (process.env.PATH || "").split(path.delimiter); + for (const dir of pathDirs) { + const cmdPath = path.join(dir, "dataform.cmd"); + if (fs.existsSync(cmdPath)) { + return cmdPath; + } + } + } + return "dataform"; +} + +function execCli(commandArgs, options = {}) { + const bin = resolveDataformCmd(); + const cwd = options.cwd || process.cwd(); + const env = { ...process.env, ...(options.env || {}) }; + + let shellCommand = ""; + let spawnFile = bin; + let spawnArgs = commandArgs; + + if (bin.endsWith(".js")) { + spawnFile = process.execPath; + spawnArgs = [path.resolve(bin), ...commandArgs]; + } + + if (targetShell === "powershell") { + spawnFile = "powershell"; + const binPart = bin.endsWith(".js") ? `"${process.execPath}" "${path.resolve(bin)}"` : `& "${bin}"`; + const quotedArgs = commandArgs.map((a) => (a.includes(" ") ? `"${a}"` : a)).join(" "); + spawnArgs = ["-NoProfile", "-NonInteractive", "-Command", `${binPart} ${quotedArgs}`]; + } else if (targetShell === "cmd") { + spawnFile = "cmd.exe"; + const binPart = bin.endsWith(".js") ? `"${process.execPath}" "${path.resolve(bin)}"` : `"${bin}"`; + const quotedArgs = commandArgs.map((a) => (a.includes(" ") ? `"${a}"` : a)).join(" "); + spawnArgs = ["/c", `${binPart} ${quotedArgs}`]; + } else if (os.platform() === "win32") { + // Default Windows spawning requires shell: true for .cmd shims + return spawnSync(bin, commandArgs, { + cwd, + env, + shell: true, + encoding: "utf8", + timeout: options.timeout || 60000, + }); + } + + return spawnSync(spawnFile, spawnArgs, { + cwd, + env, + encoding: "utf8", + timeout: options.timeout || 60000, + }); +} + +function toPosixPath(p) { + return p ? p.replace(/\\+/g, "/") : p; +} + +function normalizeLineEndings(text) { + return text ? text.replace(/\r\n/g, "\n") : text; +} + +function createTempDir(prefix = "df-smoke-") { + return fs.mkdtempSync(path.join(os.tmpdir(), prefix)); +} + +function cleanupDir(dir) { + try { + fs.rmSync(dir, { recursive: true, force: true, maxRetries: 3 }); + } catch (e) { + // Ignore cleanup errors on busy temp dirs + } +} + +// ------------------------------------------------------------- +// Suite 1: Path & Platform Unit Tests +// ------------------------------------------------------------- +async function runUnitTests() { + console.log("\n--- Suite 1: Path & Platform Unit Tests ---"); + + // We test the canonical path rules implemented in core/path.ts + await runTest("Path: canonical separator is always forward slash '/'", () => { + // Forward slash is required by internal graph contract + const canonicalSep = "/"; + assert.strictEqual(canonicalSep, "/"); + }); + + await runTest("Path: toPosixPath replaces Windows backslashes with forward slashes", () => { + assert.strictEqual(toPosixPath("definitions\\table.sqlx"), "definitions/table.sqlx"); + assert.strictEqual(toPosixPath("includes\\sub\\file.js"), "includes/sub/file.js"); + assert.strictEqual(toPosixPath("definitions\\\\nested\\\\table.sqlx"), "definitions/nested/table.sqlx"); + assert.strictEqual(toPosixPath("C:\\project\\file.sqlx"), "C:/project/file.sqlx"); + }); + + await runTest("Path: dirName handles both Windows and POSIX separators", () => { + function dirName(p) { + const normalized = toPosixPath(p); + const lastSlash = normalized.lastIndexOf("/"); + if (lastSlash === -1) return "."; + if (lastSlash === 0) return "/"; + return normalized.substring(0, lastSlash); + } + assert.strictEqual(dirName("definitions\\table.sqlx"), "definitions"); + assert.strictEqual(dirName("definitions/table.sqlx"), "definitions"); + assert.strictEqual(dirName("definitions\\nested\\table.sqlx"), "definitions/nested"); + }); + + await runTest("Path: basename handles both Windows and POSIX separators", () => { + function basename(p) { + const normalized = toPosixPath(p); + const lastSlash = normalized.lastIndexOf("/"); + return lastSlash === -1 ? normalized : normalized.substring(lastSlash + 1); + } + assert.strictEqual(basename("definitions\\table.sqlx"), "table.sqlx"); + assert.strictEqual(basename("definitions/table.sqlx"), "table.sqlx"); + }); + + await runTest("Path: relativePath handles case-insensitive Windows drive letters", () => { + function relativePath(from, to) { + let fromNorm = toPosixPath(from); + let toNorm = toPosixPath(to); + if (/^[a-zA-Z]:/.test(fromNorm) && /^[a-zA-Z]:/.test(toNorm)) { + if (fromNorm[0].toLowerCase() === toNorm[0].toLowerCase()) { + fromNorm = fromNorm[0].toLowerCase() + fromNorm.slice(1); + toNorm = toNorm[0].toLowerCase() + toNorm.slice(1); + } + } + return path.posix.relative(fromNorm, toNorm); + } + const rel = relativePath("C:/project/root", "c:/project/root/definitions/table.sqlx"); + assert.strictEqual(rel, "definitions/table.sqlx"); + }); + + await runTest("CRLF normalization preserves SQLX formatting checks", () => { + const unixFormatted = "config { type: 'table' }\nSELECT 1 AS col\n"; + const windowsFormatted = "config { type: 'table' }\r\nSELECT 1 AS col\r\n"; + assert.strictEqual(normalizeLineEndings(windowsFormatted), unixFormatted); + }); +} + +// ------------------------------------------------------------- +// Suite 2: CLI Flows +// ------------------------------------------------------------- +async function runCliFlows() { + console.log("\n--- Suite 2: CLI Smoke Tests ---"); + + let testProjectDir = null; + + await runTest("CLI Flow 1: dataform --version / help", () => { + const res = execCli(["--help"]); + assert.strictEqual(res.status, 0, `Expected exit 0, got ${res.status}. Output: ${res.stdout}\n${res.stderr}`); + assert(res.stdout.includes("dataform [command]"), "Expected output to contain 'dataform [command]'"); + }); + + await runTest("CLI Flow 2: dataform init creates project structure", () => { + testProjectDir = createTempDir("df-init-test-"); + const res = execCli(["init", testProjectDir, "test-db", "us-central1"]); + assert.strictEqual(res.status, 0, `Init failed with exit ${res.status}: ${res.stderr}\n${res.stdout}`); + + const workflowSettingsPath = path.join(testProjectDir, "workflow_settings.yaml"); + assert(fs.existsSync(workflowSettingsPath), "workflow_settings.yaml must exist"); + + let content = fs.readFileSync(workflowSettingsPath, "utf8"); + assert(content.includes("defaultProject: test-db"), "Must contain defaultProject"); + assert(content.includes("defaultLocation: us-central1"), "Must contain defaultLocation"); + + if (coreTarball) { + // Use local core tarball for offline / unpublished version testing + const tarballPosix = toPosixPath(path.resolve(coreTarball)); + content = content.replace(/dataformCoreVersion: .*/, `dataformCoreVersion: "file:${tarballPosix}"`); + fs.writeFileSync(workflowSettingsPath, content); + } + + assert(fs.existsSync(path.join(testProjectDir, "definitions")), "definitions/ folder must exist"); + assert(fs.existsSync(path.join(testProjectDir, "includes")), "includes/ folder must exist"); + }); + + await runTest("CLI Flow 3A: dataform install (workflow_settings.yaml flow)", () => { + assert(testProjectDir, "Project directory must exist"); + const res = execCli(["install", testProjectDir]); + // With workflow_settings.yaml containing dataformCoreVersion, install throws a friendly notice: + const combinedOutput = (res.stdout || "") + (res.stderr || ""); + assert( + combinedOutput.includes("No installation is needed when using workflow_settings.yaml") || + res.status === 0, + `Expected notice about workflow_settings.yaml install, got: ${combinedOutput}` + ); + }); + + await runTest("CLI Flow 3B: dataform install (package.json flow)", () => { + const pkgProjectDir = createTempDir("df-pkg-test-"); + try { + // Create dataform.json and package.json + fs.writeFileSync( + path.join(pkgProjectDir, "dataform.json"), + JSON.stringify({ + defaultDatabase: "test-db", + defaultLocation: "us-central1", + }, null, 2) + ); + + // Point @dataform/core to local tarball if provided, or a valid package + const coreDep = coreTarball ? path.resolve(coreTarball) : "^3.0.70"; + fs.writeFileSync( + path.join(pkgProjectDir, "package.json"), + JSON.stringify({ + name: "test-pkg-project", + dependencies: { + "@dataform/core": coreDep, + }, + }, null, 2) + ); + + const res = execCli(["install", pkgProjectDir]); + if (coreTarball) { + assert.strictEqual(res.status, 0, `Install failed: ${res.stderr}\n${res.stdout}`); + assert(fs.existsSync(path.join(pkgProjectDir, "node_modules")), "node_modules must exist after install"); + } else { + // If no core tarball supplied (e.g. offline run), verify it ran npm install + const output = (res.stdout || "") + (res.stderr || ""); + assert(output.includes("Installing NPM dependencies"), "Must run npm install"); + } + } finally { + cleanupDir(pkgProjectDir); + } + }); + + await runTest("CLI Flow 4A: dataform compile (Issue #1565 fix verification)", () => { + assert(testProjectDir, "Project directory must exist"); + const tableFile = path.join(testProjectDir, "definitions", "sample_table.sqlx"); + fs.writeFileSync( + tableFile, + "config {\n type: \"table\"\n}\n\nSELECT\n 1 AS sample_col\n" + ); + + const res = execCli(["compile", testProjectDir]); + assert.strictEqual(res.status, 0, `Compile failed: ${res.stderr}\n${res.stdout}`); + const output = (res.stdout || "") + (res.stderr || ""); + assert(output.includes("Compiled successfully") || output.includes("sample_table"), `Output should confirm compilation: ${output}`); + }); + + await runTest("CLI Flow 4B: dataform compile --json (Issue #510 canonical slash fix)", () => { + assert(testProjectDir, "Project directory must exist"); + const res = execCli(["compile", testProjectDir, "--json"]); + assert.strictEqual(res.status, 0, `Compile --json failed: ${res.stderr}\n${res.stdout}`); + + const compiledGraph = JSON.parse(res.stdout); + assert(Array.isArray(compiledGraph.tables), "Compiled graph must contain tables"); + const table = compiledGraph.tables.find((t) => t.target && t.target.name === "sample_table"); + assert(table, "sample_table must be present in compiled tables"); + + // Critical assertion for issue #510: fileName must use canonical forward slash '/' + assert.strictEqual(table.fileName, "definitions/sample_table.sqlx", "fileName must use canonical '/' forward slashes on Windows"); + }); + + await runTest("CLI Flow 4C: dataform compile with JiT action (declarative actions.yaml)", () => { + assert(testProjectDir, "Project directory must exist"); + const actionsYamlPath = path.join(testProjectDir, "definitions", "actions.yaml"); + fs.writeFileSync( + actionsYamlPath, + `actions:\n- notebook:\n filename: sample_notebook.ipynb\n` + ); + const notebookPath = path.join(testProjectDir, "definitions", "sample_notebook.ipynb"); + fs.writeFileSync(notebookPath, JSON.stringify({ cells: [] })); + + const res = execCli(["compile", testProjectDir, "--json"]); + assert.strictEqual(res.status, 0, `Compile with notebook failed: ${res.stderr}\n${res.stdout}`); + + const compiled = JSON.parse(res.stdout); + assert(Array.isArray(compiled.notebooks), "Compiled graph must contain notebooks"); + const nb = compiled.notebooks.find((n) => n.target && n.target.name === "sample_notebook"); + assert(nb, "sample_notebook must be compiled"); + assert.strictEqual(nb.fileName, "definitions/sample_notebook.ipynb", "notebook fileName must use '/' slashes"); + + // Clean up notebook action for subsequent tests + fs.unlinkSync(actionsYamlPath); + fs.unlinkSync(notebookPath); + }); + + await runTest("CLI Flow 4D: dataform compile --watch handles file watching", async () => { + assert(testProjectDir, "Project directory must exist"); + const bin = resolveDataformCmd(); + let spawnExe = bin; + let spawnParams = ["compile", testProjectDir, "--watch"]; + if (bin.endsWith(".js")) { + spawnExe = process.execPath; + spawnParams = [path.resolve(bin), ...spawnParams]; + } + + return new Promise((resolve, reject) => { + let watchOutput = ""; + const child = spawn(spawnExe, spawnParams, { + shell: os.platform() === "win32", + env: process.env, + }); + + let ready = false; + const timeout = setTimeout(() => { + child.kill(); + reject(new Error(`Timeout waiting for watch mode. Output was: ${watchOutput}`)); + }, 20000); + + child.stdout.on("data", (data) => { + watchOutput += data.toString(); + if ( + watchOutput.includes("ENOSPC") || + watchOutput.includes("System limit for number of file watchers reached") + ) { + clearTimeout(timeout); + child.kill(); + return resolve(); + } + if (watchOutput.includes("Watching for changes...") && !ready) { + ready = true; + // Trigger a change by updating the sample table + const tableFile = path.join(testProjectDir, "definitions", "sample_table.sqlx"); + fs.appendFileSync(tableFile, "\n-- watched change\n"); + + setTimeout(() => { + child.kill(); + clearTimeout(timeout); + fs.writeFileSync( + tableFile, + "config {\n type: \"table\"\n}\n\nSELECT\n 1 AS sample_col\n" + ); + assert(watchOutput.includes("Watching for changes..."), "Watcher started"); + resolve(); + }, 2000); + } + }); + + child.stderr.on("data", (data) => { + watchOutput += data.toString(); + if ( + watchOutput.includes("ENOSPC") || + watchOutput.includes("System limit for number of file watchers reached") + ) { + clearTimeout(timeout); + child.kill(); + return resolve(); + } + }); + + child.on("error", (err) => { + clearTimeout(timeout); + reject(err); + }); + }); + }); + + await runTest("CLI Flow 5: dataform run --dry-run --json", () => { + assert(testProjectDir, "Project directory must exist"); + // Create dummy .df-credentials.json required by dataform run/test commands + const credsPath = path.join(testProjectDir, ".df-credentials.json"); + fs.writeFileSync(credsPath, JSON.stringify({ projectId: "test-project" }, null, 2)); + + // Add an operations action which builds offline without warehouse state network calls + const opFile = path.join(testProjectDir, "definitions", "sample_operation.sqlx"); + fs.writeFileSync( + opFile, + "config {\n type: \"operations\"\n}\n\nSELECT 1 AS op_col\n" + ); + + const res = execCli(["run", testProjectDir, "--dry-run", "--json", "--actions", "sample_operation"]); + assert.strictEqual(res.status, 0, `Run dry-run failed: ${res.stderr}\n${res.stdout}`); + + const execGraph = JSON.parse(res.stdout); + assert(Array.isArray(execGraph.actions), "Execution graph must contain actions"); + const action = execGraph.actions.find((a) => a.target && a.target.name === "sample_operation"); + assert(action, "Execution graph must include sample_operation"); + + fs.unlinkSync(opFile); + }); + + await runTest("CLI Flow 6: dataform test", () => { + assert(testProjectDir, "Project directory must exist"); + // In project with no tests, exits with code 1 and 'No unit tests found.' + const resNoTests = execCli(["test", testProjectDir]); + const output = (resNoTests.stdout || "") + (resNoTests.stderr || ""); + assert(output.includes("No unit tests found."), `Must identify lack of unit tests, got: ${output}`); + + // Add a test definition + const testFile = path.join(testProjectDir, "definitions", "table_test.sqlx"); + fs.writeFileSync( + testFile, + `config {\n type: "test",\n dataset: "sample_table"\n}\n\ninput "sample_table" {\n SELECT\n 1 AS sample_col\n}\n\nSELECT\n 1 AS sample_col\n` + ); + + const resWithTest = execCli(["test", testProjectDir]); + const testOutput = (resWithTest.stdout || "") + (resWithTest.stderr || ""); + assert( + testOutput.includes("Running 1 unit test") || + testOutput.includes("Compiled successfully") || + testOutput.includes("Credentials"), + `Must compile and detect test, got: ${testOutput}` + ); + + fs.unlinkSync(testFile); + }); + + await runTest("CLI Flow 7: dataform format (with CRLF and --check)", () => { + assert(testProjectDir, "Project directory must exist"); + execCli(["format", testProjectDir]); + const formatFile = path.join(testProjectDir, "definitions", "to_format.sqlx"); + + // Write file with CRLF line endings + fs.writeFileSync(formatFile, "config {\r\n type: \"table\"\r\n}\r\n\r\nSELECT\r\n 1 AS num\r\n"); + + // Check should pass on already formatted content regardless of CRLF + const checkRes = execCli(["format", testProjectDir, "--check"]); + assert.strictEqual(checkRes.status, 0, `format --check should pass on CRLF file: ${checkRes.stderr}\n${checkRes.stdout}`); + + // Now write unformatted content + fs.writeFileSync(formatFile, "config { type: \"table\" } SELECT 1 AS num"); + const unformattedCheckRes = execCli(["format", testProjectDir, "--check"]); + assert.notStrictEqual(unformattedCheckRes.status, 0, "format --check should fail on unformatted file"); + + // Run format to fix it + const formatRes = execCli(["format", testProjectDir]); + assert.strictEqual(formatRes.status, 0, `format should succeed: ${formatRes.stderr}\n${formatRes.stdout}`); + + // Verify format --check now passes + const reCheckRes = execCli(["format", testProjectDir, "--check"]); + assert.strictEqual(reCheckRes.status, 0, "format --check must pass after running format"); + + fs.unlinkSync(formatFile); + }); + + await runTest("CLI Flow 8: dataform init-creds --help", () => { + assert(testProjectDir, "Project directory must exist"); + const res = execCli(["init-creds", "--help"]); + assert.strictEqual(res.status, 0, `init-creds --help failed: ${res.stderr}\n${res.stdout}`); + const output = (res.stdout || "") + (res.stderr || ""); + assert(output.includes(".df-credentials.json"), "Help must mention .df-credentials.json"); + assert(output.includes("--test-connection"), "Help must mention --test-connection option"); + }); + + // Final cleanup + if (testProjectDir) { + cleanupDir(testProjectDir); + } +} + +async function main() { + await runUnitTests(); + await runCliFlows(); + + console.log("\n================================================="); + console.log(`Results: ${passed} passed, ${failed} failed, ${skipped} skipped.`); + console.log("================================================="); + + process.exit(failed > 0 ? 1 : 0); +} + +main().catch((err) => { + console.error("Unhandled exception in test runner:", err); + process.exit(1); +}); From 7fc9dec549edf9afa664d60e13b2054a53c043a9 Mon Sep 17 00:00:00 2001 From: Marcin Biernacik Date: Mon, 28 Sep 2026 18:23:16 +0000 Subject: [PATCH 02/14] Fix GitHub Actions workflow syntax for Bazel and Windows runners --- .github/workflows/test.yaml | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index e8ff5ea30..ca3a44de3 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -14,17 +14,14 @@ jobs: - name: Checkout code uses: actions/checkout@v4 + - name: Setup Bazel + uses: bazelbuild/setup-bazelisk@v3 + - name: Run unit tests - uses: gcr.io/cloud-builders/bazel@sha256:aa6c5ffd0d6d37960715d9c3392ba885e6484322c81c3604a90ccfb15c07c110 - entrypoint: bash - with: - args: ./scripts/run_tests + run: ./scripts/run_tests - name: Build package tarballs - uses: gcr.io/cloud-builders/bazel@sha256:aa6c5ffd0d6d37960715d9c3392ba885e6484322c81c3604a90ccfb15c07c110 - entrypoint: bash - with: - args: bazel build //packages/@dataform/cli:package_tar //packages/@dataform/core:package_tar + run: bazel build //packages/@dataform/cli:package_tar //packages/@dataform/core:package_tar - name: Upload CLI tarball uses: actions/upload-artifact@v4 From 3753b26312ab954396e95e46f1e5f28b17332d9b Mon Sep 17 00:00:00 2001 From: Marcin Biernacik Date: Mon, 28 Sep 2026 18:46:09 +0000 Subject: [PATCH 03/14] Preserve leading slash in getContents for POSIX absolute paths --- core/session.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/session.ts b/core/session.ts index 393a94574..6440454a6 100644 --- a/core/session.ts +++ b/core/session.ts @@ -106,7 +106,9 @@ export class Session { const callerDir = Path.dirName(callerFile); const normalizedRootDir = Path.toPosixPath(this.rootDir); const resolvedPath = Path.join(callerDir, filePath); - const absolutePath = Path.normalize(Path.join(normalizedRootDir, resolvedPath)); + const leadingSlash = normalizedRootDir.startsWith("/") ? "/" : ""; + const absolutePath = + leadingSlash + Path.normalize(Path.join(normalizedRootDir, resolvedPath)); const rootDirWithSlash = normalizedRootDir.endsWith("/") ? normalizedRootDir : normalizedRootDir + "/"; From 72215042e17eeb665874037a07edd7d5325de55e Mon Sep 17 00:00:00 2001 From: Marcin Biernacik Date: Mon, 28 Sep 2026 19:03:32 +0000 Subject: [PATCH 04/14] Fix cmd.exe command spawning in Windows smoke test suite --- scripts/smoke_test_windows.js | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/scripts/smoke_test_windows.js b/scripts/smoke_test_windows.js index 4b41af67d..04ac6dcf0 100755 --- a/scripts/smoke_test_windows.js +++ b/scripts/smoke_test_windows.js @@ -99,7 +99,18 @@ function execCli(commandArgs, options = {}) { const cwd = options.cwd || process.cwd(); const env = { ...process.env, ...(options.env || {}) }; - let shellCommand = ""; + if (targetShell === "powershell") { + const binPart = bin.endsWith(".js") ? `"${process.execPath}" "${path.resolve(bin)}"` : `& "${bin}"`; + const quotedArgs = commandArgs.map((a) => (a.includes(" ") ? `"${a}"` : a)).join(" "); + const psArgs = ["-NoProfile", "-NonInteractive", "-Command", `${binPart} ${quotedArgs}`]; + return spawnSync("powershell", psArgs, { + cwd, + env, + encoding: "utf8", + timeout: options.timeout || 60000, + }); + } + let spawnFile = bin; let spawnArgs = commandArgs; @@ -108,19 +119,12 @@ function execCli(commandArgs, options = {}) { spawnArgs = [path.resolve(bin), ...commandArgs]; } - if (targetShell === "powershell") { - spawnFile = "powershell"; - const binPart = bin.endsWith(".js") ? `"${process.execPath}" "${path.resolve(bin)}"` : `& "${bin}"`; - const quotedArgs = commandArgs.map((a) => (a.includes(" ") ? `"${a}"` : a)).join(" "); - spawnArgs = ["-NoProfile", "-NonInteractive", "-Command", `${binPart} ${quotedArgs}`]; - } else if (targetShell === "cmd") { - spawnFile = "cmd.exe"; - const binPart = bin.endsWith(".js") ? `"${process.execPath}" "${path.resolve(bin)}"` : `"${bin}"`; - const quotedArgs = commandArgs.map((a) => (a.includes(" ") ? `"${a}"` : a)).join(" "); - spawnArgs = ["/c", `${binPart} ${quotedArgs}`]; - } else if (os.platform() === "win32") { - // Default Windows spawning requires shell: true for .cmd shims - return spawnSync(bin, commandArgs, { + if (os.platform() === "win32") { + // Under cmd.exe or Windows default, delegate through cmd via shell: true. + // Node handles Windows cmd.exe argument quoting correctly without escaping quotes to backslash-quotes (\"). + const safeFile = spawnFile.includes(" ") ? `"${spawnFile}"` : spawnFile; + const safeArgs = spawnArgs.map((a) => (a.includes(" ") ? `"${a}"` : a)); + return spawnSync(safeFile, safeArgs, { cwd, env, shell: true, From 2d5cf8709cb3a17a80336054b1928304793f856f Mon Sep 17 00:00:00 2001 From: Marcin Biernacik Date: Tue, 29 Sep 2026 13:57:53 +0000 Subject: [PATCH 05/14] Clean up Windows contributor guide --- contributing.md | 7 ------- 1 file changed, 7 deletions(-) diff --git a/contributing.md b/contributing.md index b662f0b2b..9e3b50bfd 100644 --- a/contributing.md +++ b/contributing.md @@ -157,13 +157,6 @@ You can run the automated Windows smoke test suite locally on Windows or in CI: ### Windows CI on GitHub Actions All pull requests run automated CI on `windows-latest` via `.github/workflows/test.yaml`. -To make the Windows test job a required status check on your repository: -1. Go to **Settings** > **Branches** > **Branch protection rules**. -2. Edit the rule for `main`. -3. Check **Require status checks to pass before merging**. -4. Search for and select: - - `Build and test (Linux)` - - `CLI smoke & unit tests (Windows)` ## The Contribution Process From a58522c70f58cb533712913f69ba3f79c95edf04 Mon Sep 17 00:00:00 2001 From: Marcin Biernacik Date: Tue, 29 Sep 2026 14:45:23 +0000 Subject: [PATCH 06/14] Pin GitHub actions to full commit SHAs for zizmor policy --- .github/workflows/test.yaml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index ca3a44de3..cc966e7c4 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -12,10 +12,10 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout code - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - name: Setup Bazel - uses: bazelbuild/setup-bazelisk@v3 + uses: bazelbuild/setup-bazelisk@b39c379c82683a5f25d34f0d062761f62693e0b2 # v3.0.0 - name: Run unit tests run: ./scripts/run_tests @@ -24,13 +24,13 @@ jobs: run: bazel build //packages/@dataform/cli:package_tar //packages/@dataform/core:package_tar - name: Upload CLI tarball - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@4cec3d8aa04e39d1a68397de0c4cd6fb9dce8ec1 # v4.6.1 with: name: dataform-cli-tarball path: bazel-bin/packages/@dataform/cli/package.tar.gz - name: Upload Core tarball - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@4cec3d8aa04e39d1a68397de0c4cd6fb9dce8ec1 # v4.6.1 with: name: dataform-core-tarball path: bazel-bin/packages/@dataform/core/package.tar.gz @@ -41,21 +41,21 @@ jobs: runs-on: windows-latest steps: - name: Checkout code - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - name: Setup Node.js - uses: actions/setup-node@v4 + uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 with: node-version: 20 - name: Download CLI tarball - uses: actions/download-artifact@v4 + uses: actions/download-artifact@cc203385981b70ca67e1cc392babf9cc229d5806 # v4.1.9 with: name: dataform-cli-tarball path: dist/cli - name: Download Core tarball - uses: actions/download-artifact@v4 + uses: actions/download-artifact@cc203385981b70ca67e1cc392babf9cc229d5806 # v4.1.9 with: name: dataform-core-tarball path: dist/core From ed5c88ab751cb414c77e9dfdfb0b3df456df9cc9 Mon Sep 17 00:00:00 2001 From: Marcin Biernacik Date: Tue, 29 Sep 2026 14:50:03 +0000 Subject: [PATCH 07/14] Format all modified files with prettier --- contributing.md | 7 +- core/session.ts | 3 +- scripts/smoke_test_windows.js | 151 ++++++++++++++++++++++++---------- 3 files changed, 114 insertions(+), 47 deletions(-) diff --git a/contributing.md b/contributing.md index 9e3b50bfd..97e3e1b72 100644 --- a/contributing.md +++ b/contributing.md @@ -120,6 +120,7 @@ prefixing them with `//:node_modules/...`. The Dataform CLI is supported on Windows 10 and 11 in both **Command Prompt (`cmd.exe`)** and **PowerShell**. ### Prerequisites & Setup + - **Node.js**: Node 18+ or Node 20+ LTS is required. - **Git line endings**: Configure git to handle CRLF/LF cleanly: ```bash @@ -132,19 +133,23 @@ The Dataform CLI is supported on Windows 10 and 11 in both **Command Prompt (`cm ``` ### Development & Path Conventions + - **Canonical Forward Slashes**: Dataform's internal graph contracts, table names, and proto file paths require canonical POSIX forward slashes (`/`). Even on Windows, definitions must resolve as `definitions/my_table.sqlx` rather than with backslashes (`\`). - **Cross-Platform Path Helpers**: Use the `Path` utility in `core/path.ts` (`Path.join`, `Path.normalize`, `Path.relativePath`, `Path.dirName`, `Path.basename`). It handles mixed separators and Windows drive letters (e.g. `C:\` vs `c:/`) automatically. ### Running Windows Smoke Tests + You can run the automated Windows smoke test suite locally on Windows or in CI: - **In PowerShell**: + ```powershell npm install -g @dataform/cli node scripts/smoke_test_windows.js --shell powershell ``` - **In Command Prompt (`cmd.exe`)**: + ```cmd npm install -g @dataform/cli node scripts\smoke_test_windows.js --shell cmd @@ -156,8 +161,8 @@ You can run the automated Windows smoke test suite locally on Windows or in CI: ``` ### Windows CI on GitHub Actions -All pull requests run automated CI on `windows-latest` via `.github/workflows/test.yaml`. +All pull requests run automated CI on `windows-latest` via `.github/workflows/test.yaml`. ## The Contribution Process diff --git a/core/session.ts b/core/session.ts index 6440454a6..049bdb750 100644 --- a/core/session.ts +++ b/core/session.ts @@ -107,8 +107,7 @@ export class Session { const normalizedRootDir = Path.toPosixPath(this.rootDir); const resolvedPath = Path.join(callerDir, filePath); const leadingSlash = normalizedRootDir.startsWith("/") ? "/" : ""; - const absolutePath = - leadingSlash + Path.normalize(Path.join(normalizedRootDir, resolvedPath)); + const absolutePath = leadingSlash + Path.normalize(Path.join(normalizedRootDir, resolvedPath)); const rootDirWithSlash = normalizedRootDir.endsWith("/") ? normalizedRootDir : normalizedRootDir + "/"; diff --git a/scripts/smoke_test_windows.js b/scripts/smoke_test_windows.js index 04ac6dcf0..4c2bdfeb3 100755 --- a/scripts/smoke_test_windows.js +++ b/scripts/smoke_test_windows.js @@ -100,7 +100,9 @@ function execCli(commandArgs, options = {}) { const env = { ...process.env, ...(options.env || {}) }; if (targetShell === "powershell") { - const binPart = bin.endsWith(".js") ? `"${process.execPath}" "${path.resolve(bin)}"` : `& "${bin}"`; + const binPart = bin.endsWith(".js") + ? `"${process.execPath}" "${path.resolve(bin)}"` + : `& "${bin}"`; const quotedArgs = commandArgs.map((a) => (a.includes(" ") ? `"${a}"` : a)).join(" "); const psArgs = ["-NoProfile", "-NonInteractive", "-Command", `${binPart} ${quotedArgs}`]; return spawnSync("powershell", psArgs, { @@ -177,7 +179,10 @@ async function runUnitTests() { await runTest("Path: toPosixPath replaces Windows backslashes with forward slashes", () => { assert.strictEqual(toPosixPath("definitions\\table.sqlx"), "definitions/table.sqlx"); assert.strictEqual(toPosixPath("includes\\sub\\file.js"), "includes/sub/file.js"); - assert.strictEqual(toPosixPath("definitions\\\\nested\\\\table.sqlx"), "definitions/nested/table.sqlx"); + assert.strictEqual( + toPosixPath("definitions\\\\nested\\\\table.sqlx"), + "definitions/nested/table.sqlx", + ); assert.strictEqual(toPosixPath("C:\\project\\file.sqlx"), "C:/project/file.sqlx"); }); @@ -237,14 +242,25 @@ async function runCliFlows() { await runTest("CLI Flow 1: dataform --version / help", () => { const res = execCli(["--help"]); - assert.strictEqual(res.status, 0, `Expected exit 0, got ${res.status}. Output: ${res.stdout}\n${res.stderr}`); - assert(res.stdout.includes("dataform [command]"), "Expected output to contain 'dataform [command]'"); + assert.strictEqual( + res.status, + 0, + `Expected exit 0, got ${res.status}. Output: ${res.stdout}\n${res.stderr}`, + ); + assert( + res.stdout.includes("dataform [command]"), + "Expected output to contain 'dataform [command]'", + ); }); await runTest("CLI Flow 2: dataform init creates project structure", () => { testProjectDir = createTempDir("df-init-test-"); const res = execCli(["init", testProjectDir, "test-db", "us-central1"]); - assert.strictEqual(res.status, 0, `Init failed with exit ${res.status}: ${res.stderr}\n${res.stdout}`); + assert.strictEqual( + res.status, + 0, + `Init failed with exit ${res.status}: ${res.stderr}\n${res.stdout}`, + ); const workflowSettingsPath = path.join(testProjectDir, "workflow_settings.yaml"); assert(fs.existsSync(workflowSettingsPath), "workflow_settings.yaml must exist"); @@ -256,11 +272,17 @@ async function runCliFlows() { if (coreTarball) { // Use local core tarball for offline / unpublished version testing const tarballPosix = toPosixPath(path.resolve(coreTarball)); - content = content.replace(/dataformCoreVersion: .*/, `dataformCoreVersion: "file:${tarballPosix}"`); + content = content.replace( + /dataformCoreVersion: .*/, + `dataformCoreVersion: "file:${tarballPosix}"`, + ); fs.writeFileSync(workflowSettingsPath, content); } - assert(fs.existsSync(path.join(testProjectDir, "definitions")), "definitions/ folder must exist"); + assert( + fs.existsSync(path.join(testProjectDir, "definitions")), + "definitions/ folder must exist", + ); assert(fs.existsSync(path.join(testProjectDir, "includes")), "includes/ folder must exist"); }); @@ -271,8 +293,8 @@ async function runCliFlows() { const combinedOutput = (res.stdout || "") + (res.stderr || ""); assert( combinedOutput.includes("No installation is needed when using workflow_settings.yaml") || - res.status === 0, - `Expected notice about workflow_settings.yaml install, got: ${combinedOutput}` + res.status === 0, + `Expected notice about workflow_settings.yaml install, got: ${combinedOutput}`, ); }); @@ -282,28 +304,39 @@ async function runCliFlows() { // Create dataform.json and package.json fs.writeFileSync( path.join(pkgProjectDir, "dataform.json"), - JSON.stringify({ - defaultDatabase: "test-db", - defaultLocation: "us-central1", - }, null, 2) + JSON.stringify( + { + defaultDatabase: "test-db", + defaultLocation: "us-central1", + }, + null, + 2, + ), ); // Point @dataform/core to local tarball if provided, or a valid package const coreDep = coreTarball ? path.resolve(coreTarball) : "^3.0.70"; fs.writeFileSync( path.join(pkgProjectDir, "package.json"), - JSON.stringify({ - name: "test-pkg-project", - dependencies: { - "@dataform/core": coreDep, + JSON.stringify( + { + name: "test-pkg-project", + dependencies: { + "@dataform/core": coreDep, + }, }, - }, null, 2) + null, + 2, + ), ); const res = execCli(["install", pkgProjectDir]); if (coreTarball) { assert.strictEqual(res.status, 0, `Install failed: ${res.stderr}\n${res.stdout}`); - assert(fs.existsSync(path.join(pkgProjectDir, "node_modules")), "node_modules must exist after install"); + assert( + fs.existsSync(path.join(pkgProjectDir, "node_modules")), + "node_modules must exist after install", + ); } else { // If no core tarball supplied (e.g. offline run), verify it ran npm install const output = (res.stdout || "") + (res.stderr || ""); @@ -317,15 +350,15 @@ async function runCliFlows() { await runTest("CLI Flow 4A: dataform compile (Issue #1565 fix verification)", () => { assert(testProjectDir, "Project directory must exist"); const tableFile = path.join(testProjectDir, "definitions", "sample_table.sqlx"); - fs.writeFileSync( - tableFile, - "config {\n type: \"table\"\n}\n\nSELECT\n 1 AS sample_col\n" - ); + fs.writeFileSync(tableFile, 'config {\n type: "table"\n}\n\nSELECT\n 1 AS sample_col\n'); const res = execCli(["compile", testProjectDir]); assert.strictEqual(res.status, 0, `Compile failed: ${res.stderr}\n${res.stdout}`); const output = (res.stdout || "") + (res.stderr || ""); - assert(output.includes("Compiled successfully") || output.includes("sample_table"), `Output should confirm compilation: ${output}`); + assert( + output.includes("Compiled successfully") || output.includes("sample_table"), + `Output should confirm compilation: ${output}`, + ); }); await runTest("CLI Flow 4B: dataform compile --json (Issue #510 canonical slash fix)", () => { @@ -339,7 +372,11 @@ async function runCliFlows() { assert(table, "sample_table must be present in compiled tables"); // Critical assertion for issue #510: fileName must use canonical forward slash '/' - assert.strictEqual(table.fileName, "definitions/sample_table.sqlx", "fileName must use canonical '/' forward slashes on Windows"); + assert.strictEqual( + table.fileName, + "definitions/sample_table.sqlx", + "fileName must use canonical '/' forward slashes on Windows", + ); }); await runTest("CLI Flow 4C: dataform compile with JiT action (declarative actions.yaml)", () => { @@ -347,7 +384,7 @@ async function runCliFlows() { const actionsYamlPath = path.join(testProjectDir, "definitions", "actions.yaml"); fs.writeFileSync( actionsYamlPath, - `actions:\n- notebook:\n filename: sample_notebook.ipynb\n` + `actions:\n- notebook:\n filename: sample_notebook.ipynb\n`, ); const notebookPath = path.join(testProjectDir, "definitions", "sample_notebook.ipynb"); fs.writeFileSync(notebookPath, JSON.stringify({ cells: [] })); @@ -359,7 +396,11 @@ async function runCliFlows() { assert(Array.isArray(compiled.notebooks), "Compiled graph must contain notebooks"); const nb = compiled.notebooks.find((n) => n.target && n.target.name === "sample_notebook"); assert(nb, "sample_notebook must be compiled"); - assert.strictEqual(nb.fileName, "definitions/sample_notebook.ipynb", "notebook fileName must use '/' slashes"); + assert.strictEqual( + nb.fileName, + "definitions/sample_notebook.ipynb", + "notebook fileName must use '/' slashes", + ); // Clean up notebook action for subsequent tests fs.unlinkSync(actionsYamlPath); @@ -410,7 +451,7 @@ async function runCliFlows() { clearTimeout(timeout); fs.writeFileSync( tableFile, - "config {\n type: \"table\"\n}\n\nSELECT\n 1 AS sample_col\n" + 'config {\n type: "table"\n}\n\nSELECT\n 1 AS sample_col\n', ); assert(watchOutput.includes("Watching for changes..."), "Watcher started"); resolve(); @@ -445,12 +486,16 @@ async function runCliFlows() { // Add an operations action which builds offline without warehouse state network calls const opFile = path.join(testProjectDir, "definitions", "sample_operation.sqlx"); - fs.writeFileSync( - opFile, - "config {\n type: \"operations\"\n}\n\nSELECT 1 AS op_col\n" - ); - - const res = execCli(["run", testProjectDir, "--dry-run", "--json", "--actions", "sample_operation"]); + fs.writeFileSync(opFile, 'config {\n type: "operations"\n}\n\nSELECT 1 AS op_col\n'); + + const res = execCli([ + "run", + testProjectDir, + "--dry-run", + "--json", + "--actions", + "sample_operation", + ]); assert.strictEqual(res.status, 0, `Run dry-run failed: ${res.stderr}\n${res.stdout}`); const execGraph = JSON.parse(res.stdout); @@ -466,22 +511,25 @@ async function runCliFlows() { // In project with no tests, exits with code 1 and 'No unit tests found.' const resNoTests = execCli(["test", testProjectDir]); const output = (resNoTests.stdout || "") + (resNoTests.stderr || ""); - assert(output.includes("No unit tests found."), `Must identify lack of unit tests, got: ${output}`); + assert( + output.includes("No unit tests found."), + `Must identify lack of unit tests, got: ${output}`, + ); // Add a test definition const testFile = path.join(testProjectDir, "definitions", "table_test.sqlx"); fs.writeFileSync( testFile, - `config {\n type: "test",\n dataset: "sample_table"\n}\n\ninput "sample_table" {\n SELECT\n 1 AS sample_col\n}\n\nSELECT\n 1 AS sample_col\n` + `config {\n type: "test",\n dataset: "sample_table"\n}\n\ninput "sample_table" {\n SELECT\n 1 AS sample_col\n}\n\nSELECT\n 1 AS sample_col\n`, ); const resWithTest = execCli(["test", testProjectDir]); const testOutput = (resWithTest.stdout || "") + (resWithTest.stderr || ""); assert( testOutput.includes("Running 1 unit test") || - testOutput.includes("Compiled successfully") || - testOutput.includes("Credentials"), - `Must compile and detect test, got: ${testOutput}` + testOutput.includes("Compiled successfully") || + testOutput.includes("Credentials"), + `Must compile and detect test, got: ${testOutput}`, ); fs.unlinkSync(testFile); @@ -493,20 +541,35 @@ async function runCliFlows() { const formatFile = path.join(testProjectDir, "definitions", "to_format.sqlx"); // Write file with CRLF line endings - fs.writeFileSync(formatFile, "config {\r\n type: \"table\"\r\n}\r\n\r\nSELECT\r\n 1 AS num\r\n"); + fs.writeFileSync( + formatFile, + 'config {\r\n type: "table"\r\n}\r\n\r\nSELECT\r\n 1 AS num\r\n', + ); // Check should pass on already formatted content regardless of CRLF const checkRes = execCli(["format", testProjectDir, "--check"]); - assert.strictEqual(checkRes.status, 0, `format --check should pass on CRLF file: ${checkRes.stderr}\n${checkRes.stdout}`); + assert.strictEqual( + checkRes.status, + 0, + `format --check should pass on CRLF file: ${checkRes.stderr}\n${checkRes.stdout}`, + ); // Now write unformatted content - fs.writeFileSync(formatFile, "config { type: \"table\" } SELECT 1 AS num"); + fs.writeFileSync(formatFile, 'config { type: "table" } SELECT 1 AS num'); const unformattedCheckRes = execCli(["format", testProjectDir, "--check"]); - assert.notStrictEqual(unformattedCheckRes.status, 0, "format --check should fail on unformatted file"); + assert.notStrictEqual( + unformattedCheckRes.status, + 0, + "format --check should fail on unformatted file", + ); // Run format to fix it const formatRes = execCli(["format", testProjectDir]); - assert.strictEqual(formatRes.status, 0, `format should succeed: ${formatRes.stderr}\n${formatRes.stdout}`); + assert.strictEqual( + formatRes.status, + 0, + `format should succeed: ${formatRes.stderr}\n${formatRes.stdout}`, + ); // Verify format --check now passes const reCheckRes = execCli(["format", testProjectDir, "--check"]); From 0c16b4d3df8daf7a8c32c48096d9404692ce7dda Mon Sep 17 00:00:00 2001 From: Marcin Biernacik Date: Tue, 29 Sep 2026 15:42:37 +0000 Subject: [PATCH 08/14] Set explicit read-only permissions and timeouts for zizmor --- .github/workflows/test.yaml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index cc966e7c4..fff7b187f 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -6,10 +6,16 @@ on: branches: [main, windows-support] workflow_dispatch: +permissions: + contents: read + jobs: test-linux: name: Build and test (Linux) runs-on: ubuntu-latest + timeout-minutes: 60 + permissions: + contents: read steps: - name: Checkout code uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 @@ -39,6 +45,9 @@ jobs: name: CLI smoke & unit tests (Windows) needs: test-linux runs-on: windows-latest + timeout-minutes: 30 + permissions: + contents: read steps: - name: Checkout code uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 From c3c213b2be6f3f36ea002b76ad04e2680468f2a6 Mon Sep 17 00:00:00 2001 From: Marcin Biernacik Date: Thu, 1 Oct 2026 21:25:01 +0000 Subject: [PATCH 09/14] Address review comments on VmRunner --- .github/workflows/test.yaml | 31 +- cli/api/commands/base_worker.ts | 2 +- cli/commands/format_command.ts | 9 +- cli/index_project_test.ts | 26 ++ cli/tests/windows/BUILD | 22 ++ cli/tests/windows/smoke_test.ts | 497 ++++++++++++++++++++++++++ contributing.md | 30 +- core/main_js_api_test.ts | 28 ++ core/main_session_test.ts | 21 ++ core/path.ts | 54 ++- core/path_test.ts | 59 +++ core/session.ts | 17 +- core/utils.ts | 3 +- core/workflow_settings.ts | 6 +- scripts/smoke_test_windows.js | 610 -------------------------------- 15 files changed, 759 insertions(+), 656 deletions(-) create mode 100644 cli/tests/windows/BUILD create mode 100644 cli/tests/windows/smoke_test.ts delete mode 100755 scripts/smoke_test_windows.js diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index fff7b187f..9382db8c3 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -3,7 +3,7 @@ name: Test on: pull_request: push: - branches: [main, windows-support] + branches: [main] workflow_dispatch: permissions: @@ -26,8 +26,12 @@ jobs: - name: Run unit tests run: ./scripts/run_tests - - name: Build package tarballs - run: bazel build //packages/@dataform/cli:package_tar //packages/@dataform/core:package_tar + - name: Build package tarballs and the Windows smoke test bundle + run: | + bazel build \ + //packages/@dataform/cli:package_tar \ + //packages/@dataform/core:package_tar \ + //cli/tests/windows:smoke_test_bundle - name: Upload CLI tarball uses: actions/upload-artifact@4cec3d8aa04e39d1a68397de0c4cd6fb9dce8ec1 # v4.6.1 @@ -41,17 +45,20 @@ jobs: name: dataform-core-tarball path: bazel-bin/packages/@dataform/core/package.tar.gz + - name: Upload Windows smoke test bundle + uses: actions/upload-artifact@4cec3d8aa04e39d1a68397de0c4cd6fb9dce8ec1 # v4.6.1 + with: + name: windows-smoke-test + path: bazel-bin/cli/tests/windows/smoke_test_bundle.js + test-windows: - name: CLI smoke & unit tests (Windows) + name: CLI smoke tests (Windows) needs: test-linux runs-on: windows-latest timeout-minutes: 30 permissions: contents: read steps: - - name: Checkout code - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - - name: Setup Node.js uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 with: @@ -69,15 +76,21 @@ jobs: name: dataform-core-tarball path: dist/core + - name: Download Windows smoke test bundle + uses: actions/download-artifact@cc203385981b70ca67e1cc392babf9cc229d5806 # v4.1.9 + with: + name: windows-smoke-test + path: dist/smoke + - name: Install @dataform/cli globally from tarball run: npm install -g dist/cli/package.tar.gz - name: Run Windows smoke tests in PowerShell shell: powershell run: | - node scripts/smoke_test_windows.js --shell powershell --core-tarball dist/core/package.tar.gz + node dist/smoke/smoke_test_bundle.js --shell powershell --core-tarball dist/core/package.tar.gz - name: Run Windows smoke tests in cmd.exe shell: cmd run: | - node scripts\smoke_test_windows.js --shell cmd --core-tarball dist\core\package.tar.gz + node dist\smoke\smoke_test_bundle.js --shell cmd --core-tarball dist\core\package.tar.gz diff --git a/cli/api/commands/base_worker.ts b/cli/api/commands/base_worker.ts index 436d9c015..11dc2b8d1 100644 --- a/cli/api/commands/base_worker.ts +++ b/cli/api/commands/base_worker.ts @@ -32,7 +32,7 @@ export abstract class BaseWorker { } completed = true; clearTimeout(timeout); - child.kill(); + child.kill("SIGKILL"); fn(); }; diff --git a/cli/commands/format_command.ts b/cli/commands/format_command.ts index 8d822f982..3c613aef5 100644 --- a/cli/commands/format_command.ts +++ b/cli/commands/format_command.ts @@ -12,7 +12,6 @@ import { } from "df/cli/common_options"; import { printError, printFormatFilesResult, printSuccess } from "df/cli/console"; import { ICommand, INamedOption } from "df/cli/yargswrapper"; -import { toPosixPath } from "df/core/path"; import { formatFile } from "df/sqlx/format"; const normalizeLineEndings = (text: string) => text.replace(/\r\n/g, "\n"); @@ -53,7 +52,13 @@ export const formatCommand: ICommand = { actions = argv.actions; } const filenames = actions - .map((action: string) => glob.sync(toPosixPath(action), { cwd: argv.projectDir })) + .map((action: string) => + glob.sync(action, { + cwd: argv.projectDir, + // Treat "\" as a path separator only on Windows; on POSIX it remains a glob escape. + windowsPathsNoEscape: process.platform === "win32", + }), + ) .flat(); const isCheckMode = argv.check; diff --git a/cli/index_project_test.ts b/cli/index_project_test.ts index 14bf61aca..ec0d47ec5 100644 --- a/cli/index_project_test.ts +++ b/cli/index_project_test.ts @@ -110,5 +110,31 @@ modules.exports = { const bufFromContents = Buffer.from(jsContents, "utf-8"); expect(bufFromContents.equals(bufFromFile)).equals(true); }); + + test("format --check accepts correctly formatted files with CRLF line endings", async () => { + const projectDir = tmpDirFixture.createNewTmpDir(); + await setupProject(tmpDirFixture, projectDir); + writeDefinitionFile( + projectDir, + "crlf.sqlx", + ` +config { type: "table" } +SELECT 1 as test +`, + ); + expect((await runCli("format", [projectDir])).exitCode).equals(0); + + // Same formatted content, but with Windows line endings (e.g. a Git checkout with + // core.autocrlf=true). + const crlfFilePath = path.join(projectDir, "definitions", "crlf.sqlx"); + const formattedContent = fs.readFileSync(crlfFilePath, "utf8"); + expect(formattedContent).not.contains("\r\n"); + fs.writeFileSync(crlfFilePath, formattedContent.replace(/\n/g, "\r\n")); + + const checkResult = await runCli("format", [projectDir, "--check"]); + + expect(checkResult.exitCode).equals(0); + expect(checkResult.stdout).contains("All files are formatted correctly"); + }); }); }); diff --git a/cli/tests/windows/BUILD b/cli/tests/windows/BUILD new file mode 100644 index 000000000..cf4d8701d --- /dev/null +++ b/cli/tests/windows/BUILD @@ -0,0 +1,22 @@ +load("//packages:index.bzl", "pkg_bundle") +load("//tools:ts_library.bzl", "ts_library") + +package(default_visibility = ["//visibility:public"]) + +ts_library( + name = "smoke_test_lib", + srcs = ["smoke_test.ts"], + deps = [ + "@npm//@types/node", + ], +) + +# Self-contained CommonJS bundle (Node built-ins only) that the Windows CI runner executes with +# plain `node`; it is built on the Linux job and shipped as a workflow artifact. +pkg_bundle( + name = "smoke_test_bundle", + allow_node_builtins = True, + entry_point = "smoke_test.ts", + externals = [], + deps = [":smoke_test_lib"], +) diff --git a/cli/tests/windows/smoke_test.ts b/cli/tests/windows/smoke_test.ts new file mode 100644 index 000000000..e0187740e --- /dev/null +++ b/cli/tests/windows/smoke_test.ts @@ -0,0 +1,497 @@ +/** + * End-to-end smoke tests for the Dataform CLI on Windows. + * + * The CLI is expected to be installed globally (`npm install -g `). Each flow is + * executed through the shell selected with `--shell` so that the `dataform.cmd` shim is exercised + * the same way a user would run it: + * + * node smoke_test_bundle.js --shell powershell [--core-tarball ] [--dataform-bin ] + * node smoke_test_bundle.js --shell cmd [--core-tarball ] [--dataform-bin ] + * + * Without `--shell` the CLI is spawned directly, which also works on Linux and macOS. + * + * `--core-tarball` installs `@dataform/core` from a local tarball through `package.json` + * (`"file:"`), the same flow used by the CLI unit tests. Without it the version written by + * `dataform init` is installed from the npm registry. + * + * This file is bundled with Bazel (`//cli/tests/windows:smoke_test_bundle`) into a single + * CommonJS file that only depends on Node built-ins, so the Windows CI runner needs nothing but + * Node.js to execute it. + */ + +import { spawn, spawnSync, SpawnSyncReturns } from "child_process"; +import * as fs from "fs"; +import * as os from "os"; +import * as path from "path"; + +type ShellMode = "cmd" | "powershell" | "direct"; + +interface IDataformBinary { + /** `dataform.cmd` shim on Windows or the `dataform` launcher elsewhere; null if unknown. */ + shimPath: string | null; + /** `node_modules/@dataform/cli/bundle.js`; used to spawn the CLI without any shell. */ + bundlePath: string | null; +} + +interface IExecOptions { + cwd?: string; + timeoutMillis?: number; +} + +const WATCH_TIMEOUT_MILLIS = 45_000; +const DEFAULT_TIMEOUT_MILLIS = 120_000; +const SAMPLE_TABLE_SQLX = 'config {\n type: "table"\n}\n\nSELECT\n 1 AS sample_col\n'; + +// --------------------------------------------------------------------------- +// Command line flags +// --------------------------------------------------------------------------- + +const argv = process.argv.slice(2); + +function getFlag(flag: string): string | null { + const index = argv.indexOf(flag); + return index !== -1 && index + 1 < argv.length ? argv[index + 1] : null; +} + +function parseShellMode(value: string | null): ShellMode { + switch (value) { + case null: + case "direct": + return "direct"; + case "cmd": + case "powershell": + if (os.platform() !== "win32") { + throw new Error(`--shell ${value} is only supported on Windows`); + } + return value; + default: + throw new Error(`Unknown --shell value "${value}"; expected cmd, powershell or direct`); + } +} + +const shellMode = parseShellMode(getFlag("--shell")); +const customBinary = getFlag("--dataform-bin") || process.env.DATAFORM_BIN || null; +const coreTarball = getFlag("--core-tarball"); + +// --------------------------------------------------------------------------- +// Minimal assertion helpers (keeps the bundle free of non built-in dependencies) +// --------------------------------------------------------------------------- + +function check(condition: boolean, message: string): void { + if (!condition) { + throw new Error(message); + } +} + +function checkEqual(actual: T, expected: T, message: string): void { + check(actual === expected, `${message}\n expected: ${expected}\n actual: ${actual}`); +} + +function checkIncludes(haystack: string, needle: string, message: string): void { + check(haystack.includes(needle), `${message}\n expected to find: ${needle}\n in:\n${haystack}`); +} + +function describeResult(result: SpawnSyncReturns): string { + return `exit=${result.status} signal=${result.signal}\nstdout:\n${result.stdout}\nstderr:\n${result.stderr}`; +} + +// --------------------------------------------------------------------------- +// Locating and invoking the CLI +// --------------------------------------------------------------------------- + +function findOnPath(executable: string): string | null { + for (const dir of (process.env.PATH || "").split(path.delimiter)) { + const candidate = path.join(dir, executable); + if (dir && fs.existsSync(candidate)) { + return candidate; + } + } + return null; +} + +function findBundleNextToShim(shimPath: string): string | null { + const shimDir = path.dirname(shimPath); + const candidates = [ + // npm global prefix on Windows: %APPDATA%\npm\dataform.cmd + %APPDATA%\npm\node_modules\... + path.join(shimDir, "node_modules", "@dataform", "cli", "bundle.js"), + // npm global prefix on POSIX: /bin/dataform + /lib/node_modules/... + path.join(shimDir, "..", "lib", "node_modules", "@dataform", "cli", "bundle.js"), + ]; + try { + // On POSIX the launcher is a symlink straight to bundle.js. + candidates.unshift(fs.realpathSync(shimPath)); + } catch (e) { + // Ignore: the shim does not exist or is not a symlink. + } + const found = candidates.find( + (candidate) => candidate.endsWith(".js") && fs.existsSync(candidate), + ); + return found ? path.resolve(found) : null; +} + +function resolveDataformBinary(): IDataformBinary { + if (customBinary) { + const resolved = path.resolve(customBinary); + if (resolved.endsWith(".js")) { + return { shimPath: null, bundlePath: resolved }; + } + return { shimPath: resolved, bundlePath: findBundleNextToShim(resolved) }; + } + const shimPath = findOnPath(os.platform() === "win32" ? "dataform.cmd" : "dataform"); + if (!shimPath) { + throw new Error( + "dataform is not on PATH; install it with `npm install -g` or pass --dataform-bin", + ); + } + return { shimPath, bundlePath: findBundleNextToShim(shimPath) }; +} + +const binary = resolveDataformBinary(); + +/** The program and leading arguments that start the CLI, e.g. `[dataform.cmd]` or `[node, bundle.js]`. */ +function cliCommand(): string[] { + if (binary.shimPath) { + return [binary.shimPath]; + } + return [process.execPath, binary.bundlePath]; +} + +function quoteForCmd(arg: string): string { + // cmd.exe has no escape for an embedded double quote; the arguments used here never need one. + check(!arg.includes('"'), `cmd.exe argument must not contain double quotes: ${arg}`); + return /[\s&|<>^()]/.test(arg) ? `"${arg}"` : arg; +} + +function quoteForPowerShell(arg: string): string { + return `'${arg.replace(/'/g, "''")}'`; +} + +function execCli(args: string[], options: IExecOptions = {}): SpawnSyncReturns { + const spawnOptions = { + cwd: options.cwd, + env: process.env, + encoding: "utf8" as const, + timeout: options.timeoutMillis || DEFAULT_TIMEOUT_MILLIS, + }; + const command = [...cliCommand(), ...args]; + switch (shellMode) { + case "powershell": + return spawnSync( + "powershell", + [ + "-NoProfile", + "-NonInteractive", + "-Command", + ["&", ...command.map(quoteForPowerShell)].join(" "), + ], + spawnOptions, + ); + case "cmd": + // The whole command line is built here and passed verbatim, so Node does not re-quote it + // (and does not emit DEP0190, which is triggered by `shell: true` with an argument array). + // `/s` makes cmd.exe strip only the outer pair of quotes. + return spawnSync("cmd.exe", ["/d", "/s", "/c", `"${command.map(quoteForCmd).join(" ")}"`], { + ...spawnOptions, + windowsVerbatimArguments: true, + }); + default: + return spawnSync(command[0], command.slice(1), spawnOptions); + } +} + +// --------------------------------------------------------------------------- +// Test runner +// --------------------------------------------------------------------------- + +let passed = 0; +let failed = 0; + +async function runTest(name: string, fn: () => void | Promise): Promise { + process.stdout.write(`TEST: ${name} ... `); + try { + await fn(); + console.log("PASSED"); + passed++; + } catch (e) { + console.log("FAILED"); + console.error(` ${e instanceof Error ? e.stack || e.message : String(e)}`); + failed++; + } +} + +function createTempDir(prefix: string): string { + return fs.mkdtempSync(path.join(os.tmpdir(), prefix)); +} + +function removeDir(dir: string): void { + // Windows may still hold handles on recently closed files; retry a few times. + fs.rmSync(dir, { recursive: true, force: true, maxRetries: 5 }); +} + +function toPosixPath(filePath: string): string { + return filePath.replace(/\\/g, "/"); +} + +// --------------------------------------------------------------------------- +// CLI flows +// --------------------------------------------------------------------------- + +async function runCliFlows(projectDir: string): Promise { + const definitionsDir = path.join(projectDir, "definitions"); + const workflowSettingsPath = path.join(projectDir, "workflow_settings.yaml"); + const sampleTablePath = path.join(definitionsDir, "sample_table.sqlx"); + + await runTest("CLI Flow 1: dataform --help", () => { + const result = execCli(["--help"]); + checkEqual(result.status, 0, `--help must exit with 0: ${describeResult(result)}`); + checkIncludes(result.stdout, "dataform [command]", "--help must print the usage line"); + }); + + await runTest("CLI Flow 2: dataform init creates the project structure", () => { + const result = execCli(["init", projectDir, "test-db", "us-central1"]); + checkEqual(result.status, 0, `init must exit with 0: ${describeResult(result)}`); + check(fs.existsSync(workflowSettingsPath), "workflow_settings.yaml must exist"); + const workflowSettings = fs.readFileSync(workflowSettingsPath, "utf8"); + checkIncludes(workflowSettings, "defaultProject: test-db", "defaultProject must be set"); + checkIncludes(workflowSettings, "defaultLocation: us-central1", "defaultLocation must be set"); + check(fs.existsSync(definitionsDir), "definitions/ must exist"); + check(fs.existsSync(path.join(projectDir, "includes")), "includes/ must exist"); + }); + + await runTest("CLI Flow 3A: dataform install refuses projects with dataformCoreVersion", () => { + const result = execCli(["install", projectDir]); + checkEqual(result.status, 1, `install must exit with 1: ${describeResult(result)}`); + checkIncludes( + result.stderr, + "No installation is needed when using workflow_settings.yaml", + "install must explain that packages are installed at runtime", + ); + }); + + await runTest("CLI Flow 3B: dataform install installs @dataform/core from package.json", () => { + // Switch the project to the package.json flow (the same setup as the CLI unit tests): the + // version pinned by `dataform init` moves from workflow_settings.yaml to package.json, or is + // replaced by the local tarball when one was provided. + const workflowSettings = fs.readFileSync(workflowSettingsPath, "utf8"); + const versionMatch = /^dataformCoreVersion:\s*(\S+)\s*$/m.exec(workflowSettings); + check(!!versionMatch, "workflow_settings.yaml written by init must pin dataformCoreVersion"); + fs.writeFileSync(workflowSettingsPath, workflowSettings.replace(versionMatch[0], "")); + const coreDependency = coreTarball + ? `file:${toPosixPath(path.resolve(coreTarball))}` + : versionMatch[1]; + fs.writeFileSync( + path.join(projectDir, "package.json"), + JSON.stringify({ dependencies: { "@dataform/core": coreDependency } }, null, 2), + ); + + const result = execCli(["install", projectDir]); + checkEqual(result.status, 0, `install must exit with 0: ${describeResult(result)}`); + check( + fs.existsSync(path.join(projectDir, "node_modules", "@dataform", "core", "package.json")), + "node_modules/@dataform/core must be installed", + ); + }); + + await runTest("CLI Flow 4A: dataform compile (issue #1565)", () => { + fs.writeFileSync(sampleTablePath, SAMPLE_TABLE_SQLX); + const result = execCli(["compile", projectDir]); + checkEqual(result.status, 0, `compile must exit with 0: ${describeResult(result)}`); + checkIncludes(result.stdout, "Compiled 1 action(s).", "compile must report one action"); + }); + + await runTest("CLI Flow 4B: dataform compile --json uses forward slashes (issue #510)", () => { + const result = execCli(["compile", projectDir, "--json"]); + checkEqual(result.status, 0, `compile --json must exit with 0: ${describeResult(result)}`); + const graph = JSON.parse(result.stdout); + check(Array.isArray(graph.tables), "compiled graph must contain tables"); + const table = graph.tables.find((t: any) => t.target && t.target.name === "sample_table"); + check(!!table, "sample_table must be in the compiled graph"); + checkEqual(table.fileName, "definitions/sample_table.sqlx", "fileName must use '/'"); + }); + + await runTest("CLI Flow 4C: dataform compile with a notebook declared in actions.yaml", () => { + const actionsYamlPath = path.join(definitionsDir, "actions.yaml"); + const notebookPath = path.join(definitionsDir, "sample_notebook.ipynb"); + fs.writeFileSync( + actionsYamlPath, + "actions:\n- notebook:\n filename: sample_notebook.ipynb\n", + ); + fs.writeFileSync(notebookPath, JSON.stringify({ cells: [] })); + try { + const result = execCli(["compile", projectDir, "--json"]); + checkEqual(result.status, 0, `compile must exit with 0: ${describeResult(result)}`); + const graph = JSON.parse(result.stdout); + check(Array.isArray(graph.notebooks), "compiled graph must contain notebooks"); + const notebook = graph.notebooks.find( + (n: any) => n.target && n.target.name === "sample_notebook", + ); + check(!!notebook, "sample_notebook must be in the compiled graph"); + checkEqual(notebook.fileName, "definitions/sample_notebook.ipynb", "fileName must use '/'"); + } finally { + fs.unlinkSync(actionsYamlPath); + fs.unlinkSync(notebookPath); + } + }); + + await runTest("CLI Flow 4D: dataform compile --watch recompiles after a change", () => { + check( + !!binary.bundlePath, + "bundle.js could not be located next to the dataform launcher; pass --dataform-bin ", + ); + return new Promise((resolve, reject) => { + // Spawn node directly, without cmd.exe or PowerShell in between, so that child.kill() + // terminates the dataform process itself rather than the shell. + const child = spawn(process.execPath, [binary.bundlePath, "compile", projectDir, "--watch"], { + env: process.env, + }); + let output = ""; + let changeWritten = false; + let finished = false; + const compiledCount = () => output.split("Compiled 1 action(s).").length - 1; + const finish = (error?: Error) => { + if (finished) { + return; + } + finished = true; + clearTimeout(timer); + child.kill(); + fs.writeFileSync(sampleTablePath, SAMPLE_TABLE_SQLX); + if (error) { + reject(error); + } else { + resolve(); + } + }; + const timer = setTimeout( + () => finish(new Error(`Timed out waiting for a recompilation. Output:\n${output}`)), + WATCH_TIMEOUT_MILLIS, + ); + const onData = (chunk: Buffer) => { + output += chunk.toString(); + if (!changeWritten && output.includes("Watching for changes...")) { + // The watcher is ready; touch a definition and wait for the second compilation. + changeWritten = true; + fs.appendFileSync(sampleTablePath, "\n-- watched change\n"); + } else if (changeWritten && compiledCount() >= 2) { + finish(); + } + }; + child.stdout.on("data", onData); + child.stderr.on("data", onData); + child.on("error", (error) => finish(error)); + child.on("exit", (code) => { + if (compiledCount() < 2) { + finish(new Error(`watch exited early with code ${code}. Output:\n${output}`)); + } + }); + }); + }); + + await runTest("CLI Flow 5: dataform run --dry-run --json", () => { + // run/test read credentials before doing anything; a dry run never contacts BigQuery. + fs.writeFileSync( + path.join(projectDir, ".df-credentials.json"), + JSON.stringify({ projectId: "test-project" }, null, 2), + ); + const operationPath = path.join(definitionsDir, "sample_operation.sqlx"); + fs.writeFileSync(operationPath, 'config {\n type: "operations"\n}\n\nSELECT 1 AS op_col\n'); + try { + const result = execCli([ + "run", + projectDir, + "--dry-run", + "--json", + "--actions", + "sample_operation", + ]); + checkEqual(result.status, 0, `run --dry-run must exit with 0: ${describeResult(result)}`); + const executionGraph = JSON.parse(result.stdout); + check(Array.isArray(executionGraph.actions), "execution graph must contain actions"); + check( + executionGraph.actions.some((a: any) => a.target && a.target.name === "sample_operation"), + "execution graph must include sample_operation", + ); + } finally { + fs.unlinkSync(operationPath); + } + }); + + await runTest("CLI Flow 6: dataform test", () => { + const noTestsResult = execCli(["test", projectDir]); + checkEqual(noTestsResult.status, 1, `test must exit with 1: ${describeResult(noTestsResult)}`); + checkIncludes(noTestsResult.stderr, "No unit tests found.", "test must report missing tests"); + + const testPath = path.join(definitionsDir, "table_test.sqlx"); + fs.writeFileSync( + testPath, + 'config {\n type: "test",\n dataset: "sample_table"\n}\n\n' + + 'input "sample_table" {\n SELECT\n 1 AS sample_col\n}\n\nSELECT\n 1 AS sample_col\n', + ); + try { + // Only compilation and test discovery are verified; executing the test needs BigQuery. + const result = execCli(["test", projectDir], { timeoutMillis: 60_000 }); + checkIncludes(result.stdout, "Running 1 unit tests...", "test must discover the unit test"); + } finally { + fs.unlinkSync(testPath); + } + }); + + await runTest("CLI Flow 7: dataform format and --check with CRLF line endings", () => { + const filePath = path.join(definitionsDir, "to_format.sqlx"); + try { + fs.writeFileSync(filePath, 'config { type: "table" } SELECT 1 AS num'); + const unformattedCheck = execCli(["format", projectDir, "--check"]); + checkEqual( + unformattedCheck.status, + 1, + `--check must fail: ${describeResult(unformattedCheck)}`, + ); + checkIncludes(unformattedCheck.stderr, "to_format.sqlx", "--check must list the file"); + + const format = execCli(["format", projectDir]); + checkEqual(format.status, 0, `format must exit with 0: ${describeResult(format)}`); + const formatted = fs.readFileSync(filePath, "utf8"); + check(!formatted.includes("\r\n"), "format must write LF line endings"); + + // The same formatted content checked out with CRLF (git core.autocrlf=true) must pass. + fs.writeFileSync(filePath, formatted.replace(/\n/g, "\r\n")); + const crlfCheck = execCli(["format", projectDir, "--check"]); + checkEqual(crlfCheck.status, 0, `--check must accept CRLF: ${describeResult(crlfCheck)}`); + checkIncludes(crlfCheck.stdout, "All files are formatted correctly", "--check must pass"); + } finally { + fs.unlinkSync(filePath); + } + }); + + await runTest("CLI Flow 8: dataform init-creds --help", () => { + const result = execCli(["init-creds", "--help"]); + checkEqual(result.status, 0, `init-creds --help must exit with 0: ${describeResult(result)}`); + checkIncludes(result.stdout, ".df-credentials.json", "help must mention the credentials file"); + checkIncludes(result.stdout, "--test-connection", "help must mention --test-connection"); + }); +} + +async function main(): Promise { + console.log("=== Dataform CLI smoke tests ==="); + console.log(`OS: ${os.platform()} ${os.release()}, Node: ${process.version}`); + console.log(`Shell: ${shellMode}`); + console.log(`Launcher: ${binary.shimPath || "(none)"}`); + console.log(`Bundle: ${binary.bundlePath || "(not found)"}`); + console.log(`Core tarball: ${coreTarball || "(none; installing from the npm registry)"}`); + console.log("================================\n"); + + const projectDir = createTempDir("df-smoke-"); + try { + await runCliFlows(projectDir); + } finally { + removeDir(projectDir); + } + + console.log(`\nResults: ${passed} passed, ${failed} failed.`); + process.exit(failed > 0 ? 1 : 0); +} + +main().catch((error) => { + console.error("Unhandled error in the smoke test runner:", error); + process.exit(1); +}); diff --git a/contributing.md b/contributing.md index 97e3e1b72..c0891b5f9 100644 --- a/contributing.md +++ b/contributing.md @@ -139,30 +139,40 @@ The Dataform CLI is supported on Windows 10 and 11 in both **Command Prompt (`cm ### Running Windows Smoke Tests -You can run the automated Windows smoke test suite locally on Windows or in CI: +The smoke test suite lives in `cli/tests/windows/smoke_test.ts` and is bundled by Bazel into a +single file that only needs Node.js to run. Build it on Linux, macOS or WSL (or download the +`windows-smoke-test` artifact from a CI run of `.github/workflows/test.yaml`): + +```bash +bazel build //cli/tests/windows:smoke_test_bundle //packages/@dataform/cli:package_tar //packages/@dataform/core:package_tar +# -> bazel-bin/cli/tests/windows/smoke_test_bundle.js +``` + +Then, on Windows, install the CLI globally and run the bundle through the shell you want to test: - **In PowerShell**: ```powershell - npm install -g @dataform/cli - node scripts/smoke_test_windows.js --shell powershell + npm install -g path\to\cli\package.tar.gz + node smoke_test_bundle.js --shell powershell --core-tarball path\to\core\package.tar.gz ``` - **In Command Prompt (`cmd.exe`)**: ```cmd - npm install -g @dataform/cli - node scripts\smoke_test_windows.js --shell cmd + npm install -g path\to\cli\package.tar.gz + node smoke_test_bundle.js --shell cmd --core-tarball path\to\core\package.tar.gz ``` -- **Testing local package tarballs**: - ```powershell - node scripts/smoke_test_windows.js --core-tarball path/to/@dataform/core/package.tar.gz - ``` +Without `--core-tarball`, the `@dataform/core` version pinned by `dataform init` is installed from +the npm registry instead. Without `--shell`, the CLI is spawned directly, which also works on +Linux and macOS. ### Windows CI on GitHub Actions -All pull requests run automated CI on `windows-latest` via `.github/workflows/test.yaml`. +All pull requests run the `CLI smoke tests (Windows)` job on `windows-latest` via +`.github/workflows/test.yaml`. It installs the CLI tarball built by the Linux job and runs the +smoke tests under both PowerShell and `cmd.exe`. ## The Contribution Process diff --git a/core/main_js_api_test.ts b/core/main_js_api_test.ts index 396bfe956..9588ad07c 100644 --- a/core/main_js_api_test.ts +++ b/core/main_js_api_test.ts @@ -1,4 +1,6 @@ import { expect } from "chai"; +import * as fs from "fs"; +import * as path from "path"; import { dataform, google } from "df/protos/ts"; import { @@ -662,6 +664,32 @@ dataform.jitData("key", {test: () => {}}); "outside the project directory", ); }); + + test("throws error for a sibling directory differing only by case", () => { + // With root ".../Proj", "../../proj/secret.md" must not be readable on case-sensitive + // file systems, even though it would be the same directory on Windows. + const parentDir = tmpDirFixture.createNewTmpDir(); + const projectDir = path.join(parentDir, "Proj"); + fs.mkdirSync(projectDir); + fs.mkdirSync(path.join(parentDir, "proj")); + fs.writeFileSync(path.join(parentDir, "proj", "secret.md"), "secret"); + writeWorkflowSettingsFile(projectDir, VALID_WORKFLOW_SETTINGS_YAML); + writeDefinitionFile( + projectDir, + "table.sqlx", + `config { + type: "table", + description: getContents('../../proj/secret.md'), + } + SELECT 1 AS test`, + ); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + + expect(result.compile.compiledGraph.graphErrors.compilationErrors[0].message).to.include( + "outside the project directory", + ); + }); }); suite("invalid options", () => { [ diff --git a/core/main_session_test.ts b/core/main_session_test.ts index 0dce5d3a1..7007471ad 100644 --- a/core/main_session_test.ts +++ b/core/main_session_test.ts @@ -279,6 +279,27 @@ actions: ]); }); + test("backslash file paths in the compile config are normalized to forward slashes", () => { + const projectDir = tmpDirFixture.createNewTmpDir(); + writeWorkflowSettingsFile(projectDir, VALID_WORKFLOW_SETTINGS_YAML); + writeDefinitionFile(projectDir, "a.sqlx", `config { type: "table" } SELECT 1 AS test`); + const coreExecutionRequest = dataform.CoreExecutionRequest.create({ + compile: { + compileConfig: { + projectDir, + filePaths: ["definitions\\a.sqlx"], + }, + }, + }); + + const result = runMainInVm(coreExecutionRequest); + + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(result.compile.compiledGraph.tables.map((table) => table.fileName)).deep.equals([ + "definitions/a.sqlx", + ]); + }); + test("fails when non-unique target", () => { const projectDir = tmpDirFixture.createNewTmpDir(); writeWorkflowSettingsFile(projectDir, VALID_WORKFLOW_SETTINGS_YAML); diff --git a/core/path.ts b/core/path.ts index 9a35450da..cc5b192be 100644 --- a/core/path.ts +++ b/core/path.ts @@ -1,22 +1,60 @@ export const separator = "/"; +/** + * Replaces every backslash with a forward slash. Repeated separators are deliberately kept, so a + * UNC root such as `\\server\share\proj` becomes `//server/share/proj` rather than a POSIX-looking + * `/server/share/proj`. + */ export function toPosixPath(path: string): string { - return path ? path.replace(/\\+/g, "/") : path; + return path ? path.replace(/\\/g, "/") : path; } +const WINDOWS_DRIVE_LETTER = /^[a-zA-Z]:(?=\/|$)/; + +/** + * Normalizes a path for comparison purposes: forward slashes and a lower-cased Windows drive + * letter, so that `C:/proj` and `c:/proj` compare equal. Everything else keeps its case, because + * POSIX file systems are case-sensitive. + */ +export function comparablePath(path: string): string { + const posixPath = toPosixPath(path); + return WINDOWS_DRIVE_LETTER.test(posixPath) + ? posixPath[0].toLowerCase() + posixPath.slice(1) + : posixPath; +} + +/** + * Returns true if `path` is `base` itself or is located inside `base`. Only Windows drive letters + * are compared case-insensitively. + */ +export function startsWithPath(path: string, base: string): boolean { + const comparable = comparablePath(path); + const comparableBase = comparablePath(base); + if (comparable === comparableBase) { + return true; + } + return comparable.startsWith( + comparableBase.endsWith("/") ? comparableBase : comparableBase + "/", + ); +} + +/** + * Returns `fullPath` relative to `base`, using forward slashes: + * - `definitions/a.sqlx` for a path inside `base`, + * - `""` when `fullPath` is `base` itself, + * - the normalized `fullPath` unchanged when `base` is empty or `fullPath` lies outside of it. + */ export function relativePath(fullPath: string, base: string) { const normalizedFull = toPosixPath(fullPath); if (base.length === 0) { return normalizedFull; } - let normalizedBase = toPosixPath(base); - if (!normalizedBase.endsWith("/")) { - normalizedBase += "/"; - } - if (normalizedFull.toLowerCase().startsWith(normalizedBase.toLowerCase())) { - return normalizedFull.slice(normalizedBase.length); + const normalizedBase = toPosixPath(base); + if (!startsWithPath(normalizedFull, normalizedBase)) { + return normalizedFull; } - return normalizedFull; + const stripped = normalizedFull.slice(normalizedBase.length); + return stripped.startsWith("/") ? stripped.slice(1) : stripped; } export function filename(path: string) { diff --git a/core/path_test.ts b/core/path_test.ts index dd837e3fe..f83576522 100644 --- a/core/path_test.ts +++ b/core/path_test.ts @@ -17,6 +17,43 @@ suite("path utils", () => { expect(Path.toPosixPath("")).equals(""); }); + test("toPosixPath keeps the UNC prefix of Windows network paths", () => { + expect(Path.toPosixPath("\\\\server\\share\\proj")).equals("//server/share/proj"); + expect(Path.toPosixPath("\\\\server\\share\\proj\\definitions\\a.sqlx")).equals( + "//server/share/proj/definitions/a.sqlx", + ); + }); + + test("comparablePath lower-cases only the Windows drive letter", () => { + expect(Path.comparablePath("C:\\Proj\\Defs")).equals("c:/Proj/Defs"); + expect(Path.comparablePath("c:/Proj")).equals("c:/Proj"); + expect(Path.comparablePath("C:")).equals("c:"); + expect(Path.comparablePath("/home/Alice")).equals("/home/Alice"); + expect(Path.comparablePath("Definitions/A.sqlx")).equals("Definitions/A.sqlx"); + // Not a drive letter: no slash (or end of string) after the colon. + expect(Path.comparablePath("C:Proj")).equals("C:Proj"); + }); + + test("startsWithPath is case-sensitive except for Windows drive letters", () => { + expect(Path.startsWithPath("/tmp/Proj/definitions/a.sqlx", "/tmp/Proj")).equals(true); + expect(Path.startsWithPath("/tmp/Proj", "/tmp/Proj")).equals(true); + expect(Path.startsWithPath("/tmp/Proj/", "/tmp/Proj")).equals(true); + expect(Path.startsWithPath("/tmp/Proj/a.sqlx", "/tmp/Proj/")).equals(true); + expect(Path.startsWithPath("/tmp/proj/secret.json", "/tmp/Proj")).equals(false); + expect(Path.startsWithPath("/tmp/Projects/a.sqlx", "/tmp/Proj")).equals(false); + expect(Path.startsWithPath("/other/a.sqlx", "/tmp/Proj")).equals(false); + expect(Path.startsWithPath("c:\\proj\\a.sqlx", "C:/proj")).equals(true); + expect(Path.startsWithPath("C:/proj/a.sqlx", "c:\\proj")).equals(true); + expect(Path.startsWithPath("C:/Proj/a.sqlx", "C:/proj")).equals(false); + expect(Path.startsWithPath("D:/proj/a.sqlx", "C:/proj")).equals(false); + expect( + Path.startsWithPath("\\\\server\\share\\proj\\a.sqlx", "\\\\server\\share\\proj"), + ).equals(true); + expect(Path.startsWithPath("/server/share/proj/a.sqlx", "\\\\server\\share\\proj")).equals( + false, + ); + }); + test("filename extracts file name with POSIX and Windows slashes", () => { expect(Path.filename("definitions/table.sqlx")).equals("table.sqlx"); expect(Path.filename("definitions\\table.sqlx")).equals("table.sqlx"); @@ -45,6 +82,9 @@ suite("path utils", () => { expect(Path.relativePath("/path/to/project/definitions/file.sqlx", "/path/to/project")).equals( "definitions/file.sqlx", ); + expect(Path.relativePath("/path/to/project/definitions/file.sqlx", "/path/to/project/")).equals( + "definitions/file.sqlx", + ); expect(Path.relativePath("C:\\project\\definitions\\file.sqlx", "C:\\project")).equals( "definitions/file.sqlx", ); @@ -57,6 +97,25 @@ suite("path utils", () => { expect(Path.relativePath("file.sqlx", "")).equals("file.sqlx"); }); + test("relativePath ignores case only in the Windows drive letter", () => { + expect(Path.relativePath("/home/Alice/definitions/a.sqlx", "/home/alice")).equals( + "/home/Alice/definitions/a.sqlx", + ); + expect(Path.relativePath("C:/Project/definitions/a.sqlx", "C:/project")).equals( + "C:/Project/definitions/a.sqlx", + ); + }); + + test("relativePath returns empty string for the base itself and the full path when outside", () => { + expect(Path.relativePath("/home/alice", "/home/alice")).equals(""); + expect(Path.relativePath("/home/alice/", "/home/alice")).equals(""); + expect(Path.relativePath("C:\\project", "c:/project")).equals(""); + expect(Path.relativePath("/other/z.sqlx", "/home/alice")).equals("/other/z.sqlx"); + expect(Path.relativePath("/home/alice-other/z.sqlx", "/home/alice")).equals( + "/home/alice-other/z.sqlx", + ); + }); + test("join combines paths with forward slashes", () => { expect(Path.join("a", "b", "c")).equals("a/b/c"); expect(Path.join("a\\b", "c\\d")).equals("a/b/c/d"); diff --git a/core/session.ts b/core/session.ts index 049bdb750..e596eb9ac 100644 --- a/core/session.ts +++ b/core/session.ts @@ -104,15 +104,14 @@ export class Session { public getContents(filePath: string): string { const callerFile = utils.getCallerFile(this.rootDir); const callerDir = Path.dirName(callerFile); - const normalizedRootDir = Path.toPosixPath(this.rootDir); - const resolvedPath = Path.join(callerDir, filePath); - const leadingSlash = normalizedRootDir.startsWith("/") ? "/" : ""; - const absolutePath = leadingSlash + Path.normalize(Path.join(normalizedRootDir, resolvedPath)); - const rootDirWithSlash = normalizedRootDir.endsWith("/") - ? normalizedRootDir - : normalizedRootDir + "/"; - - if (!absolutePath.toLowerCase().startsWith(rootDirWithSlash.toLowerCase())) { + const rootDir = Path.toPosixPath(this.rootDir); + // Keep the root's leading slashes verbatim: "/" for POSIX, "//" for UNC (\\server\share) and + // none for drive-letter paths (C:/...). Path.join and Path.normalize strip them. + const rootPrefix = /^\/*/.exec(rootDir)[0]; + const absolutePath = + rootPrefix + Path.normalize(Path.join(rootDir.slice(rootPrefix.length), callerDir, filePath)); + + if (!Path.startsWithPath(absolutePath, rootDir)) { throw new Error(`Cannot read "${filePath}": path resolves outside the project directory.`); } diff --git a/core/utils.ts b/core/utils.ts index d03ce8479..e3a550e7a 100644 --- a/core/utils.ts +++ b/core/utils.ts @@ -46,14 +46,13 @@ export function matchPatterns(patterns: string[], values: string[]) { export function getCallerFile(rootDir: string) { let lastfile: string; const stack = getCurrentStack(); - const normalizedRootDir = Path.toPosixPath(rootDir); while (stack.length) { const rawNextLastfile = stack.shift().getFileName(); if (!rawNextLastfile) { continue; } const nextLastfile = Path.toPosixPath(rawNextLastfile); - if (!nextLastfile.toLowerCase().includes(normalizedRootDir.toLowerCase())) { + if (!Path.startsWithPath(nextLastfile, rootDir)) { continue; } if (nextLastfile.includes("node_modules")) { diff --git a/core/workflow_settings.ts b/core/workflow_settings.ts index f52d471e1..033b5ef4d 100644 --- a/core/workflow_settings.ts +++ b/core/workflow_settings.ts @@ -70,11 +70,7 @@ function verifyWorkflowSettingsAsJson(workflowSettingsAsJson: object): dataform. } // The caller of Dataform Core should ensure that the correct version is installed. - if ( - !!workflowSettings.dataformCoreVersion && - workflowSettings.dataformCoreVersion !== version && - !workflowSettings.dataformCoreVersion.startsWith("file:") - ) { + if (!!workflowSettings.dataformCoreVersion && workflowSettings.dataformCoreVersion !== version) { throw Error( `Version mismatch: workflow settings specifies version ${workflowSettings.dataformCoreVersion}` + `, but ${version} was found`, diff --git a/scripts/smoke_test_windows.js b/scripts/smoke_test_windows.js deleted file mode 100755 index 4c2bdfeb3..000000000 --- a/scripts/smoke_test_windows.js +++ /dev/null @@ -1,610 +0,0 @@ -#!/usr/bin/env node -/** - * Automated smoke and unit test suite for Dataform CLI on Windows. - * - * Exercises all primary CLI flows: - * 1. Version / Help check - * 2. dataform init - * 3. dataform install (workflow_settings.yaml and package.json flows) - * 4. dataform compile (default, --json, JiT action, --watch) - * 5. dataform run --dry-run - * 6. dataform test - * 7. dataform format (with --check and CRLF handling) - * 8. dataform init-creds (--help) - * - * Can run under cmd.exe, PowerShell, or bash. - */ - -const { spawn, spawnSync } = require("child_process"); -const fs = require("fs"); -const path = require("path"); -const os = require("os"); -const assert = require("assert"); - -// Parse CLI flags -const args = process.argv.slice(2); -function getArg(flag, defaultValue = null) { - const index = args.indexOf(flag); - if (index !== -1 && index + 1 < args.length) { - return args[index + 1]; - } - return defaultValue; -} - -const targetShell = getArg("--shell", "auto"); // "powershell", "cmd", or "auto" -const customBin = getArg("--dataform-bin", process.env.DATAFORM_BIN || null); -const coreTarball = getArg("--core-tarball", null); - -console.log("=== Dataform Windows Smoke & Unit Test Runner ==="); -console.log(`OS: ${os.platform()} (${os.release()})`); -console.log(`Node: ${process.version}`); -console.log(`Shell target: ${targetShell}`); -if (customBin) console.log(`Custom dataform binary: ${customBin}`); -if (coreTarball) console.log(`Core tarball: ${coreTarball}`); -console.log("=================================================\n"); - -let passed = 0; -let failed = 0; -let skipped = 0; - -function runTest(name, fn) { - process.stdout.write(`TEST: ${name} ... `); - try { - const result = fn(); - if (result && typeof result.then === "function") { - return result - .then(() => { - console.log("PASSED"); - passed++; - }) - .catch((err) => { - console.log("FAILED"); - console.error(` Error: ${err.message || err}`); - if (err.stack) console.error(err.stack.split("\n").slice(0, 5).join("\n")); - failed++; - }); - } else { - console.log("PASSED"); - passed++; - return Promise.resolve(); - } - } catch (err) { - console.log("FAILED"); - console.error(` Error: ${err.message || err}`); - if (err.stack) console.error(err.stack.split("\n").slice(0, 5).join("\n")); - failed++; - return Promise.resolve(); - } -} - -function resolveDataformCmd() { - if (customBin) { - return customBin; - } - if (os.platform() === "win32") { - // Look for dataform.cmd in PATH - const pathDirs = (process.env.PATH || "").split(path.delimiter); - for (const dir of pathDirs) { - const cmdPath = path.join(dir, "dataform.cmd"); - if (fs.existsSync(cmdPath)) { - return cmdPath; - } - } - } - return "dataform"; -} - -function execCli(commandArgs, options = {}) { - const bin = resolveDataformCmd(); - const cwd = options.cwd || process.cwd(); - const env = { ...process.env, ...(options.env || {}) }; - - if (targetShell === "powershell") { - const binPart = bin.endsWith(".js") - ? `"${process.execPath}" "${path.resolve(bin)}"` - : `& "${bin}"`; - const quotedArgs = commandArgs.map((a) => (a.includes(" ") ? `"${a}"` : a)).join(" "); - const psArgs = ["-NoProfile", "-NonInteractive", "-Command", `${binPart} ${quotedArgs}`]; - return spawnSync("powershell", psArgs, { - cwd, - env, - encoding: "utf8", - timeout: options.timeout || 60000, - }); - } - - let spawnFile = bin; - let spawnArgs = commandArgs; - - if (bin.endsWith(".js")) { - spawnFile = process.execPath; - spawnArgs = [path.resolve(bin), ...commandArgs]; - } - - if (os.platform() === "win32") { - // Under cmd.exe or Windows default, delegate through cmd via shell: true. - // Node handles Windows cmd.exe argument quoting correctly without escaping quotes to backslash-quotes (\"). - const safeFile = spawnFile.includes(" ") ? `"${spawnFile}"` : spawnFile; - const safeArgs = spawnArgs.map((a) => (a.includes(" ") ? `"${a}"` : a)); - return spawnSync(safeFile, safeArgs, { - cwd, - env, - shell: true, - encoding: "utf8", - timeout: options.timeout || 60000, - }); - } - - return spawnSync(spawnFile, spawnArgs, { - cwd, - env, - encoding: "utf8", - timeout: options.timeout || 60000, - }); -} - -function toPosixPath(p) { - return p ? p.replace(/\\+/g, "/") : p; -} - -function normalizeLineEndings(text) { - return text ? text.replace(/\r\n/g, "\n") : text; -} - -function createTempDir(prefix = "df-smoke-") { - return fs.mkdtempSync(path.join(os.tmpdir(), prefix)); -} - -function cleanupDir(dir) { - try { - fs.rmSync(dir, { recursive: true, force: true, maxRetries: 3 }); - } catch (e) { - // Ignore cleanup errors on busy temp dirs - } -} - -// ------------------------------------------------------------- -// Suite 1: Path & Platform Unit Tests -// ------------------------------------------------------------- -async function runUnitTests() { - console.log("\n--- Suite 1: Path & Platform Unit Tests ---"); - - // We test the canonical path rules implemented in core/path.ts - await runTest("Path: canonical separator is always forward slash '/'", () => { - // Forward slash is required by internal graph contract - const canonicalSep = "/"; - assert.strictEqual(canonicalSep, "/"); - }); - - await runTest("Path: toPosixPath replaces Windows backslashes with forward slashes", () => { - assert.strictEqual(toPosixPath("definitions\\table.sqlx"), "definitions/table.sqlx"); - assert.strictEqual(toPosixPath("includes\\sub\\file.js"), "includes/sub/file.js"); - assert.strictEqual( - toPosixPath("definitions\\\\nested\\\\table.sqlx"), - "definitions/nested/table.sqlx", - ); - assert.strictEqual(toPosixPath("C:\\project\\file.sqlx"), "C:/project/file.sqlx"); - }); - - await runTest("Path: dirName handles both Windows and POSIX separators", () => { - function dirName(p) { - const normalized = toPosixPath(p); - const lastSlash = normalized.lastIndexOf("/"); - if (lastSlash === -1) return "."; - if (lastSlash === 0) return "/"; - return normalized.substring(0, lastSlash); - } - assert.strictEqual(dirName("definitions\\table.sqlx"), "definitions"); - assert.strictEqual(dirName("definitions/table.sqlx"), "definitions"); - assert.strictEqual(dirName("definitions\\nested\\table.sqlx"), "definitions/nested"); - }); - - await runTest("Path: basename handles both Windows and POSIX separators", () => { - function basename(p) { - const normalized = toPosixPath(p); - const lastSlash = normalized.lastIndexOf("/"); - return lastSlash === -1 ? normalized : normalized.substring(lastSlash + 1); - } - assert.strictEqual(basename("definitions\\table.sqlx"), "table.sqlx"); - assert.strictEqual(basename("definitions/table.sqlx"), "table.sqlx"); - }); - - await runTest("Path: relativePath handles case-insensitive Windows drive letters", () => { - function relativePath(from, to) { - let fromNorm = toPosixPath(from); - let toNorm = toPosixPath(to); - if (/^[a-zA-Z]:/.test(fromNorm) && /^[a-zA-Z]:/.test(toNorm)) { - if (fromNorm[0].toLowerCase() === toNorm[0].toLowerCase()) { - fromNorm = fromNorm[0].toLowerCase() + fromNorm.slice(1); - toNorm = toNorm[0].toLowerCase() + toNorm.slice(1); - } - } - return path.posix.relative(fromNorm, toNorm); - } - const rel = relativePath("C:/project/root", "c:/project/root/definitions/table.sqlx"); - assert.strictEqual(rel, "definitions/table.sqlx"); - }); - - await runTest("CRLF normalization preserves SQLX formatting checks", () => { - const unixFormatted = "config { type: 'table' }\nSELECT 1 AS col\n"; - const windowsFormatted = "config { type: 'table' }\r\nSELECT 1 AS col\r\n"; - assert.strictEqual(normalizeLineEndings(windowsFormatted), unixFormatted); - }); -} - -// ------------------------------------------------------------- -// Suite 2: CLI Flows -// ------------------------------------------------------------- -async function runCliFlows() { - console.log("\n--- Suite 2: CLI Smoke Tests ---"); - - let testProjectDir = null; - - await runTest("CLI Flow 1: dataform --version / help", () => { - const res = execCli(["--help"]); - assert.strictEqual( - res.status, - 0, - `Expected exit 0, got ${res.status}. Output: ${res.stdout}\n${res.stderr}`, - ); - assert( - res.stdout.includes("dataform [command]"), - "Expected output to contain 'dataform [command]'", - ); - }); - - await runTest("CLI Flow 2: dataform init creates project structure", () => { - testProjectDir = createTempDir("df-init-test-"); - const res = execCli(["init", testProjectDir, "test-db", "us-central1"]); - assert.strictEqual( - res.status, - 0, - `Init failed with exit ${res.status}: ${res.stderr}\n${res.stdout}`, - ); - - const workflowSettingsPath = path.join(testProjectDir, "workflow_settings.yaml"); - assert(fs.existsSync(workflowSettingsPath), "workflow_settings.yaml must exist"); - - let content = fs.readFileSync(workflowSettingsPath, "utf8"); - assert(content.includes("defaultProject: test-db"), "Must contain defaultProject"); - assert(content.includes("defaultLocation: us-central1"), "Must contain defaultLocation"); - - if (coreTarball) { - // Use local core tarball for offline / unpublished version testing - const tarballPosix = toPosixPath(path.resolve(coreTarball)); - content = content.replace( - /dataformCoreVersion: .*/, - `dataformCoreVersion: "file:${tarballPosix}"`, - ); - fs.writeFileSync(workflowSettingsPath, content); - } - - assert( - fs.existsSync(path.join(testProjectDir, "definitions")), - "definitions/ folder must exist", - ); - assert(fs.existsSync(path.join(testProjectDir, "includes")), "includes/ folder must exist"); - }); - - await runTest("CLI Flow 3A: dataform install (workflow_settings.yaml flow)", () => { - assert(testProjectDir, "Project directory must exist"); - const res = execCli(["install", testProjectDir]); - // With workflow_settings.yaml containing dataformCoreVersion, install throws a friendly notice: - const combinedOutput = (res.stdout || "") + (res.stderr || ""); - assert( - combinedOutput.includes("No installation is needed when using workflow_settings.yaml") || - res.status === 0, - `Expected notice about workflow_settings.yaml install, got: ${combinedOutput}`, - ); - }); - - await runTest("CLI Flow 3B: dataform install (package.json flow)", () => { - const pkgProjectDir = createTempDir("df-pkg-test-"); - try { - // Create dataform.json and package.json - fs.writeFileSync( - path.join(pkgProjectDir, "dataform.json"), - JSON.stringify( - { - defaultDatabase: "test-db", - defaultLocation: "us-central1", - }, - null, - 2, - ), - ); - - // Point @dataform/core to local tarball if provided, or a valid package - const coreDep = coreTarball ? path.resolve(coreTarball) : "^3.0.70"; - fs.writeFileSync( - path.join(pkgProjectDir, "package.json"), - JSON.stringify( - { - name: "test-pkg-project", - dependencies: { - "@dataform/core": coreDep, - }, - }, - null, - 2, - ), - ); - - const res = execCli(["install", pkgProjectDir]); - if (coreTarball) { - assert.strictEqual(res.status, 0, `Install failed: ${res.stderr}\n${res.stdout}`); - assert( - fs.existsSync(path.join(pkgProjectDir, "node_modules")), - "node_modules must exist after install", - ); - } else { - // If no core tarball supplied (e.g. offline run), verify it ran npm install - const output = (res.stdout || "") + (res.stderr || ""); - assert(output.includes("Installing NPM dependencies"), "Must run npm install"); - } - } finally { - cleanupDir(pkgProjectDir); - } - }); - - await runTest("CLI Flow 4A: dataform compile (Issue #1565 fix verification)", () => { - assert(testProjectDir, "Project directory must exist"); - const tableFile = path.join(testProjectDir, "definitions", "sample_table.sqlx"); - fs.writeFileSync(tableFile, 'config {\n type: "table"\n}\n\nSELECT\n 1 AS sample_col\n'); - - const res = execCli(["compile", testProjectDir]); - assert.strictEqual(res.status, 0, `Compile failed: ${res.stderr}\n${res.stdout}`); - const output = (res.stdout || "") + (res.stderr || ""); - assert( - output.includes("Compiled successfully") || output.includes("sample_table"), - `Output should confirm compilation: ${output}`, - ); - }); - - await runTest("CLI Flow 4B: dataform compile --json (Issue #510 canonical slash fix)", () => { - assert(testProjectDir, "Project directory must exist"); - const res = execCli(["compile", testProjectDir, "--json"]); - assert.strictEqual(res.status, 0, `Compile --json failed: ${res.stderr}\n${res.stdout}`); - - const compiledGraph = JSON.parse(res.stdout); - assert(Array.isArray(compiledGraph.tables), "Compiled graph must contain tables"); - const table = compiledGraph.tables.find((t) => t.target && t.target.name === "sample_table"); - assert(table, "sample_table must be present in compiled tables"); - - // Critical assertion for issue #510: fileName must use canonical forward slash '/' - assert.strictEqual( - table.fileName, - "definitions/sample_table.sqlx", - "fileName must use canonical '/' forward slashes on Windows", - ); - }); - - await runTest("CLI Flow 4C: dataform compile with JiT action (declarative actions.yaml)", () => { - assert(testProjectDir, "Project directory must exist"); - const actionsYamlPath = path.join(testProjectDir, "definitions", "actions.yaml"); - fs.writeFileSync( - actionsYamlPath, - `actions:\n- notebook:\n filename: sample_notebook.ipynb\n`, - ); - const notebookPath = path.join(testProjectDir, "definitions", "sample_notebook.ipynb"); - fs.writeFileSync(notebookPath, JSON.stringify({ cells: [] })); - - const res = execCli(["compile", testProjectDir, "--json"]); - assert.strictEqual(res.status, 0, `Compile with notebook failed: ${res.stderr}\n${res.stdout}`); - - const compiled = JSON.parse(res.stdout); - assert(Array.isArray(compiled.notebooks), "Compiled graph must contain notebooks"); - const nb = compiled.notebooks.find((n) => n.target && n.target.name === "sample_notebook"); - assert(nb, "sample_notebook must be compiled"); - assert.strictEqual( - nb.fileName, - "definitions/sample_notebook.ipynb", - "notebook fileName must use '/' slashes", - ); - - // Clean up notebook action for subsequent tests - fs.unlinkSync(actionsYamlPath); - fs.unlinkSync(notebookPath); - }); - - await runTest("CLI Flow 4D: dataform compile --watch handles file watching", async () => { - assert(testProjectDir, "Project directory must exist"); - const bin = resolveDataformCmd(); - let spawnExe = bin; - let spawnParams = ["compile", testProjectDir, "--watch"]; - if (bin.endsWith(".js")) { - spawnExe = process.execPath; - spawnParams = [path.resolve(bin), ...spawnParams]; - } - - return new Promise((resolve, reject) => { - let watchOutput = ""; - const child = spawn(spawnExe, spawnParams, { - shell: os.platform() === "win32", - env: process.env, - }); - - let ready = false; - const timeout = setTimeout(() => { - child.kill(); - reject(new Error(`Timeout waiting for watch mode. Output was: ${watchOutput}`)); - }, 20000); - - child.stdout.on("data", (data) => { - watchOutput += data.toString(); - if ( - watchOutput.includes("ENOSPC") || - watchOutput.includes("System limit for number of file watchers reached") - ) { - clearTimeout(timeout); - child.kill(); - return resolve(); - } - if (watchOutput.includes("Watching for changes...") && !ready) { - ready = true; - // Trigger a change by updating the sample table - const tableFile = path.join(testProjectDir, "definitions", "sample_table.sqlx"); - fs.appendFileSync(tableFile, "\n-- watched change\n"); - - setTimeout(() => { - child.kill(); - clearTimeout(timeout); - fs.writeFileSync( - tableFile, - 'config {\n type: "table"\n}\n\nSELECT\n 1 AS sample_col\n', - ); - assert(watchOutput.includes("Watching for changes..."), "Watcher started"); - resolve(); - }, 2000); - } - }); - - child.stderr.on("data", (data) => { - watchOutput += data.toString(); - if ( - watchOutput.includes("ENOSPC") || - watchOutput.includes("System limit for number of file watchers reached") - ) { - clearTimeout(timeout); - child.kill(); - return resolve(); - } - }); - - child.on("error", (err) => { - clearTimeout(timeout); - reject(err); - }); - }); - }); - - await runTest("CLI Flow 5: dataform run --dry-run --json", () => { - assert(testProjectDir, "Project directory must exist"); - // Create dummy .df-credentials.json required by dataform run/test commands - const credsPath = path.join(testProjectDir, ".df-credentials.json"); - fs.writeFileSync(credsPath, JSON.stringify({ projectId: "test-project" }, null, 2)); - - // Add an operations action which builds offline without warehouse state network calls - const opFile = path.join(testProjectDir, "definitions", "sample_operation.sqlx"); - fs.writeFileSync(opFile, 'config {\n type: "operations"\n}\n\nSELECT 1 AS op_col\n'); - - const res = execCli([ - "run", - testProjectDir, - "--dry-run", - "--json", - "--actions", - "sample_operation", - ]); - assert.strictEqual(res.status, 0, `Run dry-run failed: ${res.stderr}\n${res.stdout}`); - - const execGraph = JSON.parse(res.stdout); - assert(Array.isArray(execGraph.actions), "Execution graph must contain actions"); - const action = execGraph.actions.find((a) => a.target && a.target.name === "sample_operation"); - assert(action, "Execution graph must include sample_operation"); - - fs.unlinkSync(opFile); - }); - - await runTest("CLI Flow 6: dataform test", () => { - assert(testProjectDir, "Project directory must exist"); - // In project with no tests, exits with code 1 and 'No unit tests found.' - const resNoTests = execCli(["test", testProjectDir]); - const output = (resNoTests.stdout || "") + (resNoTests.stderr || ""); - assert( - output.includes("No unit tests found."), - `Must identify lack of unit tests, got: ${output}`, - ); - - // Add a test definition - const testFile = path.join(testProjectDir, "definitions", "table_test.sqlx"); - fs.writeFileSync( - testFile, - `config {\n type: "test",\n dataset: "sample_table"\n}\n\ninput "sample_table" {\n SELECT\n 1 AS sample_col\n}\n\nSELECT\n 1 AS sample_col\n`, - ); - - const resWithTest = execCli(["test", testProjectDir]); - const testOutput = (resWithTest.stdout || "") + (resWithTest.stderr || ""); - assert( - testOutput.includes("Running 1 unit test") || - testOutput.includes("Compiled successfully") || - testOutput.includes("Credentials"), - `Must compile and detect test, got: ${testOutput}`, - ); - - fs.unlinkSync(testFile); - }); - - await runTest("CLI Flow 7: dataform format (with CRLF and --check)", () => { - assert(testProjectDir, "Project directory must exist"); - execCli(["format", testProjectDir]); - const formatFile = path.join(testProjectDir, "definitions", "to_format.sqlx"); - - // Write file with CRLF line endings - fs.writeFileSync( - formatFile, - 'config {\r\n type: "table"\r\n}\r\n\r\nSELECT\r\n 1 AS num\r\n', - ); - - // Check should pass on already formatted content regardless of CRLF - const checkRes = execCli(["format", testProjectDir, "--check"]); - assert.strictEqual( - checkRes.status, - 0, - `format --check should pass on CRLF file: ${checkRes.stderr}\n${checkRes.stdout}`, - ); - - // Now write unformatted content - fs.writeFileSync(formatFile, 'config { type: "table" } SELECT 1 AS num'); - const unformattedCheckRes = execCli(["format", testProjectDir, "--check"]); - assert.notStrictEqual( - unformattedCheckRes.status, - 0, - "format --check should fail on unformatted file", - ); - - // Run format to fix it - const formatRes = execCli(["format", testProjectDir]); - assert.strictEqual( - formatRes.status, - 0, - `format should succeed: ${formatRes.stderr}\n${formatRes.stdout}`, - ); - - // Verify format --check now passes - const reCheckRes = execCli(["format", testProjectDir, "--check"]); - assert.strictEqual(reCheckRes.status, 0, "format --check must pass after running format"); - - fs.unlinkSync(formatFile); - }); - - await runTest("CLI Flow 8: dataform init-creds --help", () => { - assert(testProjectDir, "Project directory must exist"); - const res = execCli(["init-creds", "--help"]); - assert.strictEqual(res.status, 0, `init-creds --help failed: ${res.stderr}\n${res.stdout}`); - const output = (res.stdout || "") + (res.stderr || ""); - assert(output.includes(".df-credentials.json"), "Help must mention .df-credentials.json"); - assert(output.includes("--test-connection"), "Help must mention --test-connection option"); - }); - - // Final cleanup - if (testProjectDir) { - cleanupDir(testProjectDir); - } -} - -async function main() { - await runUnitTests(); - await runCliFlows(); - - console.log("\n================================================="); - console.log(`Results: ${passed} passed, ${failed} failed, ${skipped} skipped.`); - console.log("================================================="); - - process.exit(failed > 0 ? 1 : 0); -} - -main().catch((err) => { - console.error("Unhandled exception in test runner:", err); - process.exit(1); -}); From 5c9b266f2888f23b9c4909738093edcd9a8dcfbd Mon Sep 17 00:00:00 2001 From: Marcin Biernacik Date: Thu, 1 Oct 2026 22:08:37 +0000 Subject: [PATCH 10/14] Address review comments on VmRunner --- .gitattributes | 1 + cli/tests/windows/smoke_test.ts | 40 ++++++++++++++++++++++++--------- contributing.md | 12 ++++------ core/main_js_api_test.ts | 23 +++++++++++++++++++ core/path.ts | 23 +++++++++++-------- core/path_test.ts | 13 +++++++++++ core/utils_test.ts | 25 +++++++++++++++++++++ 7 files changed, 110 insertions(+), 27 deletions(-) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 000000000..6313b56c5 --- /dev/null +++ b/.gitattributes @@ -0,0 +1 @@ +* text=auto eol=lf diff --git a/cli/tests/windows/smoke_test.ts b/cli/tests/windows/smoke_test.ts index e0187740e..8f67ad707 100644 --- a/cli/tests/windows/smoke_test.ts +++ b/cli/tests/windows/smoke_test.ts @@ -40,7 +40,10 @@ interface IExecOptions { const WATCH_TIMEOUT_MILLIS = 45_000; const DEFAULT_TIMEOUT_MILLIS = 120_000; -const SAMPLE_TABLE_SQLX = 'config {\n type: "table"\n}\n\nSELECT\n 1 AS sample_col\n'; +const SAMPLE_DESCRIPTION = "Sample table description"; +const SAMPLE_TABLE_SQLX = + 'config {\n type: "table",\n description: getContents("./sample_description.md")\n}\n\n' + + "SELECT\n 1 AS sample_col\n"; // --------------------------------------------------------------------------- // Command line flags @@ -293,6 +296,7 @@ async function runCliFlows(projectDir: string): Promise { }); await runTest("CLI Flow 4A: dataform compile (issue #1565)", () => { + fs.writeFileSync(path.join(definitionsDir, "sample_description.md"), SAMPLE_DESCRIPTION); fs.writeFileSync(sampleTablePath, SAMPLE_TABLE_SQLX); const result = execCli(["compile", projectDir]); checkEqual(result.status, 0, `compile must exit with 0: ${describeResult(result)}`); @@ -307,6 +311,11 @@ async function runCliFlows(projectDir: string): Promise { const table = graph.tables.find((t: any) => t.target && t.target.name === "sample_table"); check(!!table, "sample_table must be in the compiled graph"); checkEqual(table.fileName, "definitions/sample_table.sqlx", "fileName must use '/'"); + checkEqual( + table.actionDescriptor && table.actionDescriptor.description, + SAMPLE_DESCRIPTION, + "getContents must resolve relative paths inside definitions/", + ); }); await runTest("CLI Flow 4C: dataform compile with a notebook declared in actions.yaml", () => { @@ -328,8 +337,8 @@ async function runCliFlows(projectDir: string): Promise { check(!!notebook, "sample_notebook must be in the compiled graph"); checkEqual(notebook.fileName, "definitions/sample_notebook.ipynb", "fileName must use '/'"); } finally { - fs.unlinkSync(actionsYamlPath); - fs.unlinkSync(notebookPath); + fs.rmSync(actionsYamlPath, { force: true }); + fs.rmSync(notebookPath, { force: true }); } }); @@ -355,7 +364,9 @@ async function runCliFlows(projectDir: string): Promise { finished = true; clearTimeout(timer); child.kill(); - fs.writeFileSync(sampleTablePath, SAMPLE_TABLE_SQLX); + if (changeWritten) { + fs.writeFileSync(sampleTablePath, SAMPLE_TABLE_SQLX); + } if (error) { reject(error); } else { @@ -412,7 +423,7 @@ async function runCliFlows(projectDir: string): Promise { "execution graph must include sample_operation", ); } finally { - fs.unlinkSync(operationPath); + fs.rmSync(operationPath, { force: true }); } }); @@ -432,15 +443,24 @@ async function runCliFlows(projectDir: string): Promise { const result = execCli(["test", projectDir], { timeoutMillis: 60_000 }); checkIncludes(result.stdout, "Running 1 unit tests...", "test must discover the unit test"); } finally { - fs.unlinkSync(testPath); + fs.rmSync(testPath, { force: true }); } }); await runTest("CLI Flow 7: dataform format and --check with CRLF line endings", () => { const filePath = path.join(definitionsDir, "to_format.sqlx"); + // Pass the relative path with the OS separator ("definitions\to_format.sqlx" on Windows) to + // verify that `--actions` treats "\" as a path separator rather than a glob escape on Windows. + const relativeActionPath = path.join("definitions", "to_format.sqlx"); try { fs.writeFileSync(filePath, 'config { type: "table" } SELECT 1 AS num'); - const unformattedCheck = execCli(["format", projectDir, "--check"]); + const unformattedCheck = execCli([ + "format", + projectDir, + "--check", + "--actions", + relativeActionPath, + ]); checkEqual( unformattedCheck.status, 1, @@ -448,18 +468,18 @@ async function runCliFlows(projectDir: string): Promise { ); checkIncludes(unformattedCheck.stderr, "to_format.sqlx", "--check must list the file"); - const format = execCli(["format", projectDir]); + const format = execCli(["format", projectDir, "--actions", relativeActionPath]); checkEqual(format.status, 0, `format must exit with 0: ${describeResult(format)}`); const formatted = fs.readFileSync(filePath, "utf8"); check(!formatted.includes("\r\n"), "format must write LF line endings"); // The same formatted content checked out with CRLF (git core.autocrlf=true) must pass. fs.writeFileSync(filePath, formatted.replace(/\n/g, "\r\n")); - const crlfCheck = execCli(["format", projectDir, "--check"]); + const crlfCheck = execCli(["format", projectDir, "--check", "--actions", relativeActionPath]); checkEqual(crlfCheck.status, 0, `--check must accept CRLF: ${describeResult(crlfCheck)}`); checkIncludes(crlfCheck.stdout, "All files are formatted correctly", "--check must pass"); } finally { - fs.unlinkSync(filePath); + fs.rmSync(filePath, { force: true }); } }); diff --git a/contributing.md b/contributing.md index c0891b5f9..0f6b167fe 100644 --- a/contributing.md +++ b/contributing.md @@ -122,20 +122,16 @@ The Dataform CLI is supported on Windows 10 and 11 in both **Command Prompt (`cm ### Prerequisites & Setup - **Node.js**: Node 18+ or Node 20+ LTS is required. -- **Git line endings**: Configure git to handle CRLF/LF cleanly: +- **Git line endings**: This repository includes a `.gitattributes` file (`* text=auto eol=lf`) that enforces LF line endings automatically without changing your global Git configuration. For user Dataform projects checked out with CRLF (`core.autocrlf=true`), `dataform format` and `dataform format --check` are CRLF-aware and normalize line endings before comparing. +- **Symlinks & Long Paths**: If using Bazel or deep `node_modules` hierarchies on Windows, enable [Windows Developer Mode](https://learn.microsoft.com/en-us/windows/apps/get-started/enable-your-device-for-development) or enable `core.longpaths` for this repository: ```bash - git config --global core.autocrlf true - ``` - Dataform format commands (`dataform format` / `dataform format --check`) are CRLF-aware and normalize line endings before checking. -- **Symlinks & Long Paths**: If using Bazel or deep `node_modules` hierarchies on Windows, enable [Windows Developer Mode](https://learn.microsoft.com/en-us/windows/apps/get-started/enable-your-device-for-development) or enable `core.longpaths` in Git: - ```bash - git config --global core.longpaths true + git config core.longpaths true ``` ### Development & Path Conventions - **Canonical Forward Slashes**: Dataform's internal graph contracts, table names, and proto file paths require canonical POSIX forward slashes (`/`). Even on Windows, definitions must resolve as `definitions/my_table.sqlx` rather than with backslashes (`\`). -- **Cross-Platform Path Helpers**: Use the `Path` utility in `core/path.ts` (`Path.join`, `Path.normalize`, `Path.relativePath`, `Path.dirName`, `Path.basename`). It handles mixed separators and Windows drive letters (e.g. `C:\` vs `c:/`) automatically. +- **Cross-Platform Path Helpers**: Use the `Path` utility in `core/path.ts` (`Path.toPosixPath`, `Path.startsWithPath`, `Path.join`, `Path.normalize`, `Path.relativePath`, `Path.dirName`, `Path.basename`). It handles mixed separators and Windows drive letters (e.g. `C:\` vs `c:/`) automatically. ### Running Windows Smoke Tests diff --git a/core/main_js_api_test.ts b/core/main_js_api_test.ts index 9588ad07c..625c05356 100644 --- a/core/main_js_api_test.ts +++ b/core/main_js_api_test.ts @@ -625,6 +625,29 @@ dataform.jitData("key", {test: () => {}}); ); }); + test("getContents resolves parent-relative paths that remain inside rootDir", () => { + const projectDir = tmpDirFixture.createNewTmpDir(); + writeWorkflowSettingsFile(projectDir, VALID_WORKFLOW_SETTINGS_YAML); + fs.mkdirSync(path.join(projectDir, "docs")); + fs.writeFileSync(path.join(projectDir, "docs", "descriptions.md"), "# Shared description"); + writeDefinitionFile( + projectDir, + "table.sqlx", + `config { + type: "table", + description: getContents('../docs/descriptions.md'), + } + SELECT 1 AS test`, + ); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(result.compile.compiledGraph.tables[0].actionDescriptor.description).to.equal( + `# Shared description`, + ); + }); + test("throws error for invalid missing markedown", () => { const projectDir = tmpDirFixture.createNewTmpDir(); writeWorkflowSettingsFile(projectDir, VALID_WORKFLOW_SETTINGS_YAML); diff --git a/core/path.ts b/core/path.ts index cc5b192be..ee64273bf 100644 --- a/core/path.ts +++ b/core/path.ts @@ -28,13 +28,12 @@ export function comparablePath(path: string): string { * are compared case-insensitively. */ export function startsWithPath(path: string, base: string): boolean { - const comparable = comparablePath(path); - const comparableBase = comparablePath(base); - if (comparable === comparableBase) { - return true; + if (!path || !base) { + return false; } - return comparable.startsWith( - comparableBase.endsWith("/") ? comparableBase : comparableBase + "/", + const withTrailingSlash = (p: string) => (p.endsWith("/") ? p : p + "/"); + return withTrailingSlash(comparablePath(path)).startsWith( + withTrailingSlash(comparablePath(base)), ); } @@ -64,13 +63,19 @@ export function filename(path: string) { export function basename(path: string) { const f = filename(path); const dotIndex = f.lastIndexOf("."); - return dotIndex === -1 ? f : f.substring(0, dotIndex); + return dotIndex <= 0 ? f : f.substring(0, dotIndex); } export function dirName(fullPath: string) { const normalized = toPosixPath(fullPath); const lastSlash = normalized.lastIndexOf("/"); - return lastSlash === -1 ? "" : normalized.slice(0, lastSlash); + if (lastSlash === -1) { + return ""; + } + if (lastSlash === 0 || /^[a-zA-Z]:$/.test(normalized.slice(0, lastSlash))) { + return normalized.slice(0, lastSlash + 1); + } + return normalized.slice(0, lastSlash); } export function join(...paths: string[]) { @@ -96,7 +101,7 @@ export function escapedBasename(path: string) { export function fileExtension(fullPath: string) { const f = filename(fullPath); const dotIndex = f.lastIndexOf("."); - return dotIndex === -1 ? "" : f.slice(dotIndex + 1); + return dotIndex <= 0 ? "" : f.slice(dotIndex + 1); } export function normalize(path: string) { diff --git a/core/path_test.ts b/core/path_test.ts index f83576522..b74571488 100644 --- a/core/path_test.ts +++ b/core/path_test.ts @@ -38,10 +38,13 @@ suite("path utils", () => { expect(Path.startsWithPath("/tmp/Proj/definitions/a.sqlx", "/tmp/Proj")).equals(true); expect(Path.startsWithPath("/tmp/Proj", "/tmp/Proj")).equals(true); expect(Path.startsWithPath("/tmp/Proj/", "/tmp/Proj")).equals(true); + expect(Path.startsWithPath("/tmp/Proj", "/tmp/Proj/")).equals(true); expect(Path.startsWithPath("/tmp/Proj/a.sqlx", "/tmp/Proj/")).equals(true); expect(Path.startsWithPath("/tmp/proj/secret.json", "/tmp/Proj")).equals(false); expect(Path.startsWithPath("/tmp/Projects/a.sqlx", "/tmp/Proj")).equals(false); expect(Path.startsWithPath("/other/a.sqlx", "/tmp/Proj")).equals(false); + expect(Path.startsWithPath("/tmp/Proj", "")).equals(false); + expect(Path.startsWithPath("", "/tmp/Proj")).equals(false); expect(Path.startsWithPath("c:\\proj\\a.sqlx", "C:/proj")).equals(true); expect(Path.startsWithPath("C:/proj/a.sqlx", "c:\\proj")).equals(true); expect(Path.startsWithPath("C:/Proj/a.sqlx", "C:/proj")).equals(false); @@ -69,12 +72,18 @@ suite("path utils", () => { expect(Path.basename("C:\\Users\\runner\\project\\definitions\\table.sqlx")).equals("table"); expect(Path.basename("no_ext")).equals("no_ext"); expect(Path.basename("multi.dot.name.sqlx")).equals("multi.dot.name"); + expect(Path.basename(".gitignore")).equals(".gitignore"); + expect(Path.basename("definitions/.gitignore")).equals(".gitignore"); + expect(Path.basename(".df-credentials.json")).equals(".df-credentials"); }); test("dirName extracts directory path with POSIX and Windows slashes", () => { expect(Path.dirName("definitions/table.sqlx")).equals("definitions"); expect(Path.dirName("definitions\\table.sqlx")).equals("definitions"); expect(Path.dirName("C:\\project\\definitions\\table.sqlx")).equals("C:/project/definitions"); + expect(Path.dirName("/table.sqlx")).equals("/"); + expect(Path.dirName("C:\\table.sqlx")).equals("C:/"); + expect(Path.dirName("C:/table.sqlx")).equals("C:/"); expect(Path.dirName("table.sqlx")).equals(""); }); @@ -109,6 +118,7 @@ suite("path utils", () => { test("relativePath returns empty string for the base itself and the full path when outside", () => { expect(Path.relativePath("/home/alice", "/home/alice")).equals(""); expect(Path.relativePath("/home/alice/", "/home/alice")).equals(""); + expect(Path.relativePath("/home/alice", "/home/alice/")).equals(""); expect(Path.relativePath("C:\\project", "c:/project")).equals(""); expect(Path.relativePath("/other/z.sqlx", "/home/alice")).equals("/other/z.sqlx"); expect(Path.relativePath("/home/alice-other/z.sqlx", "/home/alice")).equals( @@ -126,6 +136,9 @@ suite("path utils", () => { expect(Path.fileExtension("definitions/file.sqlx")).equals("sqlx"); expect(Path.fileExtension("definitions\\file.sqlx")).equals("sqlx"); expect(Path.fileExtension("no_extension")).equals(""); + expect(Path.fileExtension(".gitignore")).equals(""); + expect(Path.fileExtension("definitions/.gitignore")).equals(""); + expect(Path.fileExtension(".df-credentials.json")).equals("json"); }); test("normalize handles both slash types and dot navigation", () => { diff --git a/core/utils_test.ts b/core/utils_test.ts index 1b16b8e72..26e2dde82 100644 --- a/core/utils_test.ts +++ b/core/utils_test.ts @@ -1,5 +1,6 @@ import { expect } from "chai"; import { + getCallerFile, getConnectionForIcebergTable, getEffectiveBucketName, getEffectiveTableFolderRoot, @@ -360,4 +361,28 @@ suite("Dataform Utility Validations", () => { ); }); }); + + suite("getCallerFile", () => { + test("returns a relative POSIX path for Windows paths with differing drive-letter case", () => { + const globalAny = global as any; + const previousCurrentFile = globalAny.__dataform_current_file; + try { + globalAny.__dataform_current_file = "c:\\project\\definitions\\sub\\table.sqlx"; + expect(getCallerFile("C:\\project")).to.equal("definitions/sub/table.sqlx"); + } finally { + globalAny.__dataform_current_file = previousCurrentFile; + } + }); + + test("returns a relative POSIX path for Windows UNC paths", () => { + const globalAny = global as any; + const previousCurrentFile = globalAny.__dataform_current_file; + try { + globalAny.__dataform_current_file = "\\\\server\\share\\proj\\definitions\\table.sqlx"; + expect(getCallerFile("\\\\server\\share\\proj")).to.equal("definitions/table.sqlx"); + } finally { + globalAny.__dataform_current_file = previousCurrentFile; + } + }); + }); }); From 6e8753df1a29df832cba23ec230803e2f2cff840 Mon Sep 17 00:00:00 2001 From: Marcin Biernacik Date: Fri, 9 Oct 2026 08:36:01 +0000 Subject: [PATCH 11/14] Address review comments --- .github/workflows/test.yaml | 11 +++++------ cli/tests/windows/smoke_test.ts | 31 +++++++++++++++++++++++-------- contributing.md | 6 +++--- core/compilers.ts | 2 +- core/path.ts | 4 ---- 5 files changed, 32 insertions(+), 22 deletions(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 9382db8c3..4c327faa2 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -10,8 +10,10 @@ permissions: contents: read jobs: - test-linux: - name: Build and test (Linux) + # Lint, unit and integration tests run on Cloud Build (cloudbuild-test.yaml); this job only + # builds the packages and the smoke test bundle that the Windows job needs. + build-linux: + name: Build packages and smoke test bundle (Linux) runs-on: ubuntu-latest timeout-minutes: 60 permissions: @@ -23,9 +25,6 @@ jobs: - name: Setup Bazel uses: bazelbuild/setup-bazelisk@b39c379c82683a5f25d34f0d062761f62693e0b2 # v3.0.0 - - name: Run unit tests - run: ./scripts/run_tests - - name: Build package tarballs and the Windows smoke test bundle run: | bazel build \ @@ -53,7 +52,7 @@ jobs: test-windows: name: CLI smoke tests (Windows) - needs: test-linux + needs: build-linux runs-on: windows-latest timeout-minutes: 30 permissions: diff --git a/cli/tests/windows/smoke_test.ts b/cli/tests/windows/smoke_test.ts index 8f67ad707..c1102ee71 100644 --- a/cli/tests/windows/smoke_test.ts +++ b/cli/tests/windows/smoke_test.ts @@ -8,7 +8,8 @@ * node smoke_test_bundle.js --shell powershell [--core-tarball ] [--dataform-bin ] * node smoke_test_bundle.js --shell cmd [--core-tarball ] [--dataform-bin ] * - * Without `--shell` the CLI is spawned directly, which also works on Linux and macOS. + * Without `--shell`, `bundle.js` is run directly with Node (no shell and no launcher), which also + * works on Linux and macOS. * * `--core-tarball` installs `@dataform/core` from a local tarball through `package.json` * (`"file:"`), the same flow used by the CLI unit tests. Without it the version written by @@ -150,10 +151,21 @@ function resolveDataformBinary(): IDataformBinary { } const binary = resolveDataformBinary(); +if (shellMode === "direct" && !binary.bundlePath) { + throw new Error( + "Direct mode runs bundle.js with node, but it could not be located next to the dataform " + + "launcher; pass --dataform-bin ", + ); +} -/** The program and leading arguments that start the CLI, e.g. `[dataform.cmd]` or `[node, bundle.js]`. */ +/** + * The program and leading arguments that start the CLI. The cmd.exe and PowerShell modes go + * through the launcher (e.g. `dataform.cmd`), as a user would. Direct mode runs + * `[node, bundle.js]`, because Node refuses to spawn a `.cmd` file without a shell (EINVAL since + * the CVE-2024-27980 fix). + */ function cliCommand(): string[] { - if (binary.shimPath) { + if (shellMode !== "direct" && binary.shimPath) { return [binary.shimPath]; } return [process.execPath, binary.bundlePath]; @@ -244,6 +256,14 @@ async function runCliFlows(projectDir: string): Promise { const workflowSettingsPath = path.join(projectDir, "workflow_settings.yaml"); const sampleTablePath = path.join(definitionsDir, "sample_table.sqlx"); + // `run` and `test` read credentials before doing anything else. Neither the dry run (Flow 5) + // nor test discovery (Flow 6) contacts BigQuery, so placeholder credentials are enough. `init` + // only refuses directories that already contain a project, so the file can be written first. + fs.writeFileSync( + path.join(projectDir, ".df-credentials.json"), + JSON.stringify({ projectId: "test-project" }, null, 2), + ); + await runTest("CLI Flow 1: dataform --help", () => { const result = execCli(["--help"]); checkEqual(result.status, 0, `--help must exit with 0: ${describeResult(result)}`); @@ -399,11 +419,6 @@ async function runCliFlows(projectDir: string): Promise { }); await runTest("CLI Flow 5: dataform run --dry-run --json", () => { - // run/test read credentials before doing anything; a dry run never contacts BigQuery. - fs.writeFileSync( - path.join(projectDir, ".df-credentials.json"), - JSON.stringify({ projectId: "test-project" }, null, 2), - ); const operationPath = path.join(definitionsDir, "sample_operation.sqlx"); fs.writeFileSync(operationPath, 'config {\n type: "operations"\n}\n\nSELECT 1 AS op_col\n'); try { diff --git a/contributing.md b/contributing.md index 0f6b167fe..2817d0d2d 100644 --- a/contributing.md +++ b/contributing.md @@ -121,7 +121,7 @@ The Dataform CLI is supported on Windows 10 and 11 in both **Command Prompt (`cm ### Prerequisites & Setup -- **Node.js**: Node 18+ or Node 20+ LTS is required. +- **Node.js**: Node 20+ LTS is required. - **Git line endings**: This repository includes a `.gitattributes` file (`* text=auto eol=lf`) that enforces LF line endings automatically without changing your global Git configuration. For user Dataform projects checked out with CRLF (`core.autocrlf=true`), `dataform format` and `dataform format --check` are CRLF-aware and normalize line endings before comparing. - **Symlinks & Long Paths**: If using Bazel or deep `node_modules` hierarchies on Windows, enable [Windows Developer Mode](https://learn.microsoft.com/en-us/windows/apps/get-started/enable-your-device-for-development) or enable `core.longpaths` for this repository: ```bash @@ -161,8 +161,8 @@ Then, on Windows, install the CLI globally and run the bundle through the shell ``` Without `--core-tarball`, the `@dataform/core` version pinned by `dataform init` is installed from -the npm registry instead. Without `--shell`, the CLI is spawned directly, which also works on -Linux and macOS. +the npm registry instead. Without `--shell`, the CLI bundle is run directly with Node (bypassing +the `dataform.cmd` launcher), which also works on Linux and macOS. ### Windows CI on GitHub Actions diff --git a/core/compilers.ts b/core/compilers.ts index 38cf3823c..9ed83b980 100644 --- a/core/compilers.ts +++ b/core/compilers.ts @@ -68,7 +68,7 @@ function compileSqlx(rootNode: SyntaxTreeNode, path: string): string { return `dataform.sqlxAction({ sqlxConfig: { - name: "${Path.escapedBasename(path)}", + name: "${Path.basename(path)}", type: "operations", ...${config || "{}"} }, diff --git a/core/path.ts b/core/path.ts index ee64273bf..16e24f8e5 100644 --- a/core/path.ts +++ b/core/path.ts @@ -94,10 +94,6 @@ export function join(...paths: string[]) { .join(separator); } -export function escapedBasename(path: string) { - return basename(path).replace(/\\/g, "\\\\"); -} - export function fileExtension(fullPath: string) { const f = filename(fullPath); const dotIndex = f.lastIndexOf("."); From 64df2b8191ef8bda65e54b63186ee16e787c4430 Mon Sep 17 00:00:00 2001 From: Marcin Biernacik Date: Mon, 12 Oct 2026 11:03:41 +0000 Subject: [PATCH 12/14] Address review comments - Emit SQLX action names with JSON.stringify and test a file name containing a double quote. - Document that toPosixPath treats backslashes as separators on every platform, and pin fileExtension's file-name-only behaviour in tests. - Keep matching every caller file when rootDir is empty, as before. - Renormalize the only CRLF file after adding .gitattributes. - Run the Windows smoke tests on Node 22 and 24 (Node 20 is EOL) and update contributing.md accordingly. - Clarify that CI always uses the local core tarball in the smoke tests. --- .github/workflows/test.yaml | 9 +- cli/tests/windows/smoke_test.ts | 5 +- contributing.md | 9 +- core/compilers.ts | 2 +- core/compilers_test.ts | 5 + core/path.ts | 8 +- core/path_test.ts | 3 + core/utils.ts | 3 +- core/utils_test.ts | 13 +++ vscode/workflow_settings_yaml.schema.json | 114 +++++++++++----------- 10 files changed, 101 insertions(+), 70 deletions(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 4c327faa2..74a4d804d 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -51,17 +51,22 @@ jobs: path: bazel-bin/cli/tests/windows/smoke_test_bundle.js test-windows: - name: CLI smoke tests (Windows) + name: CLI smoke tests (Windows, Node ${{ matrix.node-version }}) needs: build-linux runs-on: windows-latest timeout-minutes: 30 + strategy: + fail-fast: false + matrix: + # Oldest supported Node.js LTS and the current one (also used by the Bazel toolchain). + node-version: [22, 24] permissions: contents: read steps: - name: Setup Node.js uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 with: - node-version: 20 + node-version: ${{ matrix.node-version }} - name: Download CLI tarball uses: actions/download-artifact@cc203385981b70ca67e1cc392babf9cc229d5806 # v4.1.9 diff --git a/cli/tests/windows/smoke_test.ts b/cli/tests/windows/smoke_test.ts index c1102ee71..36c467156 100644 --- a/cli/tests/windows/smoke_test.ts +++ b/cli/tests/windows/smoke_test.ts @@ -12,8 +12,9 @@ * works on Linux and macOS. * * `--core-tarball` installs `@dataform/core` from a local tarball through `package.json` - * (`"file:"`), the same flow used by the CLI unit tests. Without it the version written by - * `dataform init` is installed from the npm registry. + * (`"file:"`), the same flow used by the CLI unit tests; CI always passes it. Note that + * `dataform init` installs nothing, it only pins the CLI's own version. Without `--core-tarball` + * (local runs), Flow 3B installs that pinned version from the npm registry. * * This file is bundled with Bazel (`//cli/tests/windows:smoke_test_bundle`) into a single * CommonJS file that only depends on Node built-ins, so the Windows CI runner needs nothing but diff --git a/contributing.md b/contributing.md index 2817d0d2d..0fae0b8ef 100644 --- a/contributing.md +++ b/contributing.md @@ -121,7 +121,7 @@ The Dataform CLI is supported on Windows 10 and 11 in both **Command Prompt (`cm ### Prerequisites & Setup -- **Node.js**: Node 20+ LTS is required. +- **Node.js**: Node 22+ LTS is required. - **Git line endings**: This repository includes a `.gitattributes` file (`* text=auto eol=lf`) that enforces LF line endings automatically without changing your global Git configuration. For user Dataform projects checked out with CRLF (`core.autocrlf=true`), `dataform format` and `dataform format --check` are CRLF-aware and normalize line endings before comparing. - **Symlinks & Long Paths**: If using Bazel or deep `node_modules` hierarchies on Windows, enable [Windows Developer Mode](https://learn.microsoft.com/en-us/windows/apps/get-started/enable-your-device-for-development) or enable `core.longpaths` for this repository: ```bash @@ -166,9 +166,10 @@ the `dataform.cmd` launcher), which also works on Linux and macOS. ### Windows CI on GitHub Actions -All pull requests run the `CLI smoke tests (Windows)` job on `windows-latest` via -`.github/workflows/test.yaml`. It installs the CLI tarball built by the Linux job and runs the -smoke tests under both PowerShell and `cmd.exe`. +All pull requests run the `CLI smoke tests (Windows, Node 22)` and +`CLI smoke tests (Windows, Node 24)` jobs on `windows-latest` via `.github/workflows/test.yaml`. +Each installs the CLI tarball built by the Linux job and runs the smoke tests under both PowerShell +and `cmd.exe`, with `@dataform/core` installed from the locally built tarball. ## The Contribution Process diff --git a/core/compilers.ts b/core/compilers.ts index 9ed83b980..bc87242d7 100644 --- a/core/compilers.ts +++ b/core/compilers.ts @@ -68,7 +68,7 @@ function compileSqlx(rootNode: SyntaxTreeNode, path: string): string { return `dataform.sqlxAction({ sqlxConfig: { - name: "${Path.basename(path)}", + name: ${JSON.stringify(Path.basename(path))}, type: "operations", ...${config || "{}"} }, diff --git a/core/compilers_test.ts b/core/compilers_test.ts index d5e526552..7481582b6 100644 --- a/core/compilers_test.ts +++ b/core/compilers_test.ts @@ -16,6 +16,11 @@ suite("core/compilers", () => { expect(result).to.include("select 1"); }); + test("escapes the action name derived from the file name", () => { + const result = compile("select 1", 'definitions/a"b.sqlx'); + expect(result).to.include('name: "a\\"b"'); + }); + test("compiles yml to js", () => { const code = "foo: bar"; const path = "definitions/foo.yml"; diff --git a/core/path.ts b/core/path.ts index 16e24f8e5..1aded435b 100644 --- a/core/path.ts +++ b/core/path.ts @@ -1,9 +1,11 @@ export const separator = "/"; /** - * Replaces every backslash with a forward slash. Repeated separators are deliberately kept, so a - * UNC root such as `\\server\share\proj` becomes `//server/share/proj` rather than a POSIX-looking - * `/server/share/proj`. + * Replaces every backslash with a forward slash, on every platform, so the compiled graph is the + * same on Windows and POSIX. As a result, a literal backslash in a POSIX file name (e.g. + * `definitions/a\b.sqlx`) is treated as a separator; such files can't be checked out on Windows + * anyway. Repeated separators are deliberately kept, so a UNC root such as `\\server\share\proj` + * becomes `//server/share/proj` rather than a POSIX-looking `/server/share/proj`. */ export function toPosixPath(path: string): string { return path ? path.replace(/\\/g, "/") : path; diff --git a/core/path_test.ts b/core/path_test.ts index b74571488..9c9a77c81 100644 --- a/core/path_test.ts +++ b/core/path_test.ts @@ -139,6 +139,9 @@ suite("path utils", () => { expect(Path.fileExtension(".gitignore")).equals(""); expect(Path.fileExtension("definitions/.gitignore")).equals(""); expect(Path.fileExtension(".df-credentials.json")).equals("json"); + // Only the file name is inspected, not dots in directory names. + expect(Path.fileExtension("definitions.v2/no_extension")).equals(""); + expect(Path.fileExtension("definitions.v2/file.sqlx")).equals("sqlx"); }); test("normalize handles both slash types and dot navigation", () => { diff --git a/core/utils.ts b/core/utils.ts index e3a550e7a..c4c09f391 100644 --- a/core/utils.ts +++ b/core/utils.ts @@ -52,7 +52,8 @@ export function getCallerFile(rootDir: string) { continue; } const nextLastfile = Path.toPosixPath(rawNextLastfile); - if (!Path.startsWithPath(nextLastfile, rootDir)) { + // An empty rootDir (a host that doesn't set projectDir) matches every file, as before. + if (rootDir && !Path.startsWithPath(nextLastfile, rootDir)) { continue; } if (nextLastfile.includes("node_modules")) { diff --git a/core/utils_test.ts b/core/utils_test.ts index 26e2dde82..c03a08b3c 100644 --- a/core/utils_test.ts +++ b/core/utils_test.ts @@ -384,5 +384,18 @@ suite("Dataform Utility Validations", () => { globalAny.__dataform_current_file = previousCurrentFile; } }); + + test("accepts any caller file when rootDir is empty", () => { + const globalAny = global as any; + const previousCurrentFile = globalAny.__dataform_current_file; + try { + // Hosts that don't set projectDir pass an empty rootDir; the stack must still be searched + // instead of failing with "Unable to find valid caller file". + globalAny.__dataform_current_file = undefined; + expect(() => getCallerFile("")).to.not.throw(); + } finally { + globalAny.__dataform_current_file = previousCurrentFile; + } + }); }); }); diff --git a/vscode/workflow_settings_yaml.schema.json b/vscode/workflow_settings_yaml.schema.json index 89e76da0f..37a2a95db 100644 --- a/vscode/workflow_settings_yaml.schema.json +++ b/vscode/workflow_settings_yaml.schema.json @@ -1,58 +1,58 @@ -{ - "$schema": "http://json-schema.org/draft-07/schema#", - "type": "object", - "$comment": "Created from 'protos/configs.proto'. Even options are specified as 'required' in proto, they might be specified from the CLI options, so leave them optional except for the dataformCoreVersion which is required in compile.", - "additionalProperties": false, - "properties": { - "dataformCoreVersion": { - "type": "string", - "description": "The desired dataform core version to compile against." - }, - "defaultProject": { - "type": "string", - "description": "The default Google Cloud project (database)." - }, - "defaultDataset": { - "type": "string", - "description": "The default dataset (schema)." - }, - "defaultLocation": { - "type": "string", - "description": "The default BigQuery location to use." - }, - "defaultAssertionDataset": { - "type": "string", - "description": "The default dataset (schema) for assertions." - }, - "vars": { - "type": "object", - "description": "User-defined variables that are made available to project code during compilation. An object containing a list of key-value pairs.", - "additionalProperties": { - "type": "string" - } - }, - "projectSuffix": { - "type": "string", - "description": "The suffix to append to all Google Cloud project references." - }, - "datasetSuffix": { - "type": "string", - "description": "The suffix to append to all dataset references." - }, - "namePrefix": { - "type": "string", - "description": "The prefix to append to all action names." - }, - "defaultNotebookRuntimeOptions": { - "type": "object", - "description": "Default runtime options for Notebook actions.", - "outputBucket": { - "type": "string", - "description": "Storage bucket to output notebooks to after their execution." - } - } - }, - "required": [ - "dataformCoreVersion" - ] +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "$comment": "Created from 'protos/configs.proto'. Even options are specified as 'required' in proto, they might be specified from the CLI options, so leave them optional except for the dataformCoreVersion which is required in compile.", + "additionalProperties": false, + "properties": { + "dataformCoreVersion": { + "type": "string", + "description": "The desired dataform core version to compile against." + }, + "defaultProject": { + "type": "string", + "description": "The default Google Cloud project (database)." + }, + "defaultDataset": { + "type": "string", + "description": "The default dataset (schema)." + }, + "defaultLocation": { + "type": "string", + "description": "The default BigQuery location to use." + }, + "defaultAssertionDataset": { + "type": "string", + "description": "The default dataset (schema) for assertions." + }, + "vars": { + "type": "object", + "description": "User-defined variables that are made available to project code during compilation. An object containing a list of key-value pairs.", + "additionalProperties": { + "type": "string" + } + }, + "projectSuffix": { + "type": "string", + "description": "The suffix to append to all Google Cloud project references." + }, + "datasetSuffix": { + "type": "string", + "description": "The suffix to append to all dataset references." + }, + "namePrefix": { + "type": "string", + "description": "The prefix to append to all action names." + }, + "defaultNotebookRuntimeOptions": { + "type": "object", + "description": "Default runtime options for Notebook actions.", + "outputBucket": { + "type": "string", + "description": "Storage bucket to output notebooks to after their execution." + } + } + }, + "required": [ + "dataformCoreVersion" + ] } \ No newline at end of file From def5b00055b0d017ceca6fb128f4001f864776d8 Mon Sep 17 00:00:00 2001 From: Marcin Biernacik Date: Mon, 12 Oct 2026 11:33:52 +0000 Subject: [PATCH 13/14] Adapt the Windows smoke test targets to the aspect_rules_js toolchain. --- cli/tests/windows/BUILD | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/tests/windows/BUILD b/cli/tests/windows/BUILD index cf4d8701d..00443b257 100644 --- a/cli/tests/windows/BUILD +++ b/cli/tests/windows/BUILD @@ -7,7 +7,7 @@ ts_library( name = "smoke_test_lib", srcs = ["smoke_test.ts"], deps = [ - "@npm//@types/node", + "//:node_modules/@types/node", ], ) @@ -16,7 +16,7 @@ ts_library( pkg_bundle( name = "smoke_test_bundle", allow_node_builtins = True, - entry_point = "smoke_test.ts", + entry_point = "smoke_test.js", externals = [], deps = [":smoke_test_lib"], ) From 7e64a6819a2152c21215bd91763594505e0fa093 Mon Sep 17 00:00:00 2001 From: Marcin Biernacik Date: Mon, 12 Oct 2026 11:45:37 +0000 Subject: [PATCH 14/14] Create an empty ~/.npmrc for Bazel in the Linux CI job. --- .github/workflows/test.yaml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 74a4d804d..049dcdb7c 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -25,6 +25,11 @@ jobs: - name: Setup Bazel uses: bazelbuild/setup-bazelisk@b39c379c82683a5f25d34f0d062761f62693e0b2 # v3.0.0 + # MODULE.bazel sets `use_home_npmrc = True`, so aspect_rules_js requires ~/.npmrc to exist. + # An empty file keeps the default public npm registry (Cloud Build writes its own). + - name: Create an empty ~/.npmrc + run: touch ~/.npmrc + - name: Build package tarballs and the Windows smoke test bundle run: | bazel build \