|
| 1 | +// v0.9.5 review — Tech Lead P1 (initial pass) + coderabbit / cubic follow-ups. |
| 2 | +// |
| 3 | +// sample-setup.ts::redactPaths has a docstring listing three specific bugs the |
| 4 | +// implementation was written to fix (regex swallowing surrounding sentence, |
| 5 | +// `/root/…` prefix leaking, José/O'Connor surnames leaking on the first pass). |
| 6 | +// None of them had test coverage this release. This file asserts the fixes so a |
| 7 | +// future edit to the regex or the known-value substitution loop can't silently |
| 8 | +// re-introduce them. |
| 9 | +// |
| 10 | +// countSampleContents is a tiny counting helper — but it's called on every |
| 11 | +// sample_setup invocation and its output feeds a telemetry event, so a |
| 12 | +// silent-zero bug (typo in the extension filter, wrong subdirectory name) |
| 13 | +// would misreport onboarding activity. The fixture below builds a real dir tree |
| 14 | +// per test and asserts the count. |
| 15 | +// |
| 16 | +// Fixture ownership note (bot-review follow-up, coderabbit + cubic): |
| 17 | +// countSampleContents originally shared one `mkdtempSync` at module scope with |
| 18 | +// afterAll cleanup. That leaks if the suite is filtered (only redactPaths tests |
| 19 | +// selected) or if a `beforeAll` throws before `afterAll` registers. Switched to |
| 20 | +// the repo's `await using tmp = await tmpdir()` pattern so each test owns its |
| 21 | +// fixture and cleanup is bound to the test scope. |
| 22 | + |
| 23 | +import { describe, expect, test } from "bun:test" |
| 24 | +import fs from "fs" |
| 25 | +import path from "path" |
| 26 | + |
| 27 | +import { tmpdir } from "../fixture/fixture" |
| 28 | +import { redactPaths, countSampleContents } from "../../src/altimate/tools/sample-setup" |
| 29 | + |
| 30 | +describe("redactPaths", () => { |
| 31 | + test("redacts an absolute POSIX path", () => { |
| 32 | + const out = redactPaths("failed at /usr/local/bin/dbt") |
| 33 | + expect(out).toBe("failed at <path>") |
| 34 | + }) |
| 35 | + |
| 36 | + test("redacts a Windows drive path", () => { |
| 37 | + const out = redactPaths("failed at C:\\Users\\alice\\dbt.exe") |
| 38 | + // Windows paths pattern matches `C:\` opener and consumes until whitespace/quote. |
| 39 | + expect(out).toBe("failed at <path>") |
| 40 | + expect(out).not.toContain("Users") |
| 41 | + expect(out).not.toContain("alice") |
| 42 | + }) |
| 43 | + |
| 44 | + test("redacts a home-relative path (~/)", () => { |
| 45 | + // Note: `~/.altimate/…` is redacted by the POSIX-`/` pattern first, which |
| 46 | + // starts at the leading slash and leaves the `~` as a harmless prefix. |
| 47 | + // What the assertion cares about is that no path segment leaks — the exact |
| 48 | + // shape of the redaction marker is secondary. |
| 49 | + const out = redactPaths("cannot read ~/.altimate/machine-id") |
| 50 | + expect(out).not.toContain(".altimate") |
| 51 | + expect(out).not.toContain("machine-id") |
| 52 | + expect(out).toContain("<path>") |
| 53 | + }) |
| 54 | + |
| 55 | + test("redacts a bare tilde-only path (~/x with no preceding slash)", () => { |
| 56 | + // The dedicated `~\/…` pattern is what catches this shape — the POSIX-`/` |
| 57 | + // one starts inside the path and can leave a `~` behind. |
| 58 | + const input = "opening ~/opt/dbt for read" |
| 59 | + const out = redactPaths(input) |
| 60 | + expect(out).not.toContain("opt/dbt") |
| 61 | + expect(out).toContain("<path>") |
| 62 | + }) |
| 63 | + |
| 64 | + test("terminates at whitespace, does NOT swallow the surrounding sentence", () => { |
| 65 | + // The docstring calls out this exact class of bug: an early implementation |
| 66 | + // used a character class that included `.`, so the regex would consume the |
| 67 | + // rest of the sentence past the path. Reader ends up with just "<path>". |
| 68 | + const input = "Underlying error: /Users/alice/projects/dbt-demo failed to compile" |
| 69 | + const out = redactPaths(input) |
| 70 | + expect(out).toContain("failed to compile") |
| 71 | + expect(out).toContain("Underlying error:") |
| 72 | + expect(out).not.toContain("/Users") |
| 73 | + }) |
| 74 | + |
| 75 | + test("terminates at a double-quote", () => { |
| 76 | + const out = redactPaths('opening "/Users/alice/dbt_project.yml" for read') |
| 77 | + expect(out).toContain("for read") |
| 78 | + expect(out).not.toContain("alice") |
| 79 | + }) |
| 80 | + |
| 81 | + test("handles a path containing an apostrophe (O'Connor)", () => { |
| 82 | + // Docstring bug: an early character class excluded `'`, so the regex would |
| 83 | + // stop at the apostrophe and leak the substring after it. Now apostrophes |
| 84 | + // are permitted inside the redacted run. |
| 85 | + const out = redactPaths("failed at /Users/O'Connor/projects/x") |
| 86 | + expect(out).not.toContain("O'Connor") |
| 87 | + expect(out).not.toContain("Connor") |
| 88 | + expect(out).toBe("failed at <path>") |
| 89 | + }) |
| 90 | + |
| 91 | + test("handles a path containing an accented character (José)", () => { |
| 92 | + const out = redactPaths("failed at /Users/José/dbt") |
| 93 | + expect(out).not.toContain("José") |
| 94 | + expect(out).toBe("failed at <path>") |
| 95 | + }) |
| 96 | + |
| 97 | + test("path segment terminates at the first whitespace, so a path containing a space leaks the tail", () => { |
| 98 | + // codex-review gap: the greedy pattern stops at the first `\s`, so a real |
| 99 | + // CWD like `/Users/alice/My Documents/dbt` gets split — only `/Users/alice/My` |
| 100 | + // is redacted; `Documents/dbt` is left in the output. |
| 101 | + // |
| 102 | + // The `extra` list is what production callers use to close this gap (they |
| 103 | + // pass the exact CWD to `redactPaths(msg, [cwd])`), so this test also |
| 104 | + // asserts the compensating behavior — with the CWD known-value, the whole |
| 105 | + // path collapses cleanly. |
| 106 | + const cwd = "/Users/alice/My Documents/dbt" |
| 107 | + const raw = redactPaths(`failed at ${cwd}/models/foo.sql`) |
| 108 | + // Documented limitation of the pattern-only pass: the greedy path pattern |
| 109 | + // terminates at the first whitespace, so `/Users/alice/My` and |
| 110 | + // `/dbt/models/foo.sql` each redact cleanly but the middle segment |
| 111 | + // `Documents` sits between two `<path>` markers. |
| 112 | + expect(raw).toContain("Documents") |
| 113 | + expect(raw).not.toBe("failed at <path>") |
| 114 | + // With the CWD passed as a known value the whole path collapses cleanly: |
| 115 | + const guarded = redactPaths(`failed at ${cwd}/models/foo.sql`, [cwd]) |
| 116 | + expect(guarded).toBe("failed at <path>") |
| 117 | + expect(guarded).not.toContain("Documents") |
| 118 | + expect(guarded).not.toContain("alice") |
| 119 | + }) |
| 120 | + |
| 121 | + test("collapses adjacent <path> segments so double-redaction reads clean", () => { |
| 122 | + // The known-value pass replaces os.homedir() etc first; the greedy pattern |
| 123 | + // then may match the "<path>" tail and re-redact. The collapse rule keeps |
| 124 | + // the output from becoming "<path><path><path>". |
| 125 | + const home = require("os").homedir() |
| 126 | + const out = redactPaths(`failed at ${home}/dbt/models/foo.sql`) |
| 127 | + expect(out).toBe("failed at <path>") |
| 128 | + expect(out).not.toMatch(/<path><path>/) |
| 129 | + }) |
| 130 | + |
| 131 | + test("passes short/empty known values without exploding", () => { |
| 132 | + // Guard for `known.length < 2` — empty string or single-char known values |
| 133 | + // used to `split("")` and shatter every character. The guard keeps them out |
| 134 | + // of the substitution loop. |
| 135 | + const out = redactPaths("hello world", ["", "a", undefined]) |
| 136 | + expect(out).toBe("hello world") |
| 137 | + }) |
| 138 | + |
| 139 | + test("substitutes user-supplied extras", () => { |
| 140 | + const out = redactPaths("clone failed at /tmp/checkout-xyz", ["/tmp/checkout-xyz"]) |
| 141 | + expect(out).toBe("clone failed at <path>") |
| 142 | + }) |
| 143 | + |
| 144 | + test("returns the message unchanged when nothing path-shaped is present", () => { |
| 145 | + expect(redactPaths("dbt run completed in 3s")).toBe("dbt run completed in 3s") |
| 146 | + }) |
| 147 | +}) |
| 148 | + |
| 149 | +// Shared helper — each test uses its own tmp dir via `await using`, so cleanup |
| 150 | +// is scoped to the test itself (bot-review follow-up). |
| 151 | +async function seedSampleTree(dir: string) { |
| 152 | + fs.mkdirSync(path.join(dir, "models", "staging"), { recursive: true }) |
| 153 | + fs.mkdirSync(path.join(dir, "models", "marts", "core"), { recursive: true }) |
| 154 | + fs.mkdirSync(path.join(dir, "seeds"), { recursive: true }) |
| 155 | + |
| 156 | + fs.writeFileSync(path.join(dir, "models", "top.sql"), "select 1") |
| 157 | + fs.writeFileSync(path.join(dir, "models", "staging", "stg_orders.sql"), "select 1") |
| 158 | + fs.writeFileSync(path.join(dir, "models", "staging", "stg_users.sql"), "select 1") |
| 159 | + fs.writeFileSync(path.join(dir, "models", "marts", "core", "dim_customers.sql"), "select 1") |
| 160 | + |
| 161 | + // Non-.sql alongside .sql, must NOT be counted. |
| 162 | + fs.writeFileSync(path.join(dir, "models", "readme.md"), "hi") |
| 163 | + fs.writeFileSync(path.join(dir, "models", "schema.yml"), "version: 2") |
| 164 | + |
| 165 | + fs.writeFileSync(path.join(dir, "seeds", "country_codes.csv"), "code,name\n") |
| 166 | + fs.writeFileSync(path.join(dir, "seeds", "regions.csv"), "id,name\n") |
| 167 | + // Non-.csv seed — not counted. |
| 168 | + fs.writeFileSync(path.join(dir, "seeds", "notes.md"), "hi") |
| 169 | +} |
| 170 | + |
| 171 | +describe("countSampleContents", () => { |
| 172 | + test("counts .sql files recursively under models/", async () => { |
| 173 | + await using tmp = await tmpdir() |
| 174 | + await seedSampleTree(tmp.path) |
| 175 | + expect(countSampleContents(tmp.path).models).toBe(4) |
| 176 | + }) |
| 177 | + |
| 178 | + test("counts .csv files under seeds/ (top level; matches production sample layout)", async () => { |
| 179 | + await using tmp = await tmpdir() |
| 180 | + await seedSampleTree(tmp.path) |
| 181 | + expect(countSampleContents(tmp.path).tables).toBe(2) |
| 182 | + }) |
| 183 | + |
| 184 | + test("returns zeros when the sample dir is missing the expected subdirs", async () => { |
| 185 | + await using tmp = await tmpdir() |
| 186 | + expect(countSampleContents(tmp.path)).toEqual({ models: 0, tables: 0 }) |
| 187 | + }) |
| 188 | + |
| 189 | + test("returns zeros when the sample dir does not exist at all", async () => { |
| 190 | + // countFilesWithExtension swallows the readdirSync error and returns 0 — |
| 191 | + // this is the graceful-degradation shape the telemetry event depends on. |
| 192 | + await using tmp = await tmpdir() |
| 193 | + expect(countSampleContents(path.join(tmp.path, "does-not-exist"))).toEqual({ models: 0, tables: 0 }) |
| 194 | + }) |
| 195 | +}) |
0 commit comments