From 9ffde0cccaf02758d65199b0452e731a128a24f9 Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Wed, 15 Jul 2026 18:20:42 +0200 Subject: [PATCH 1/9] Normalize execution target paths to absolute at the engine entry point fileExecutionEngineAndTarget accepted both relative and absolute paths, so the cached ExecutionTarget could hold either form depending on the caller. Its fileInformationCache keys are normalized to absolute, so when preview seeds the shared context via format resolution with a cwd-relative path and renderProject later looks the file up by its absolute path, the two collide on the same key and the render inherits the stale relative source/input. The knitr engine runs its R subprocess with cwd set to the project dir, where a subdirectory-relative filename does not resolve, so rmarkdown:::abs_path(input) throws. Normalizing the path once at this single convergence point makes target.source and target.input absolute for every caller and engine, consistent with the cache key and with project renders (which already pass absolute paths). This establishes the always-absolute contract discussed in #14151. Fixes #14683 (knitr preview of a document in a project subdirectory, run from that subdirectory with a bare filename, as RStudio's Render button invokes it). Fixes #12401 (QUARTO_DOCUMENT_PATH was relative for single-file renders but absolute for project renders). --- .claude/rules/testing/test-anti-patterns.md | 6 + llm-docs/testing-patterns.md | 34 +++ news/changelog-1.10.md | 2 + src/execute/engine.ts | 15 +- tests/unit/preview-subdir-knitr-cwd.test.ts | 245 ++++++++++++++++++ .../unit/single-file-input-abs-12401.test.ts | 69 +++++ 6 files changed, 370 insertions(+), 1 deletion(-) create mode 100644 tests/unit/preview-subdir-knitr-cwd.test.ts create mode 100644 tests/unit/single-file-input-abs-12401.test.ts diff --git a/.claude/rules/testing/test-anti-patterns.md b/.claude/rules/testing/test-anti-patterns.md index ab18275a715..e7523f9028e 100644 --- a/.claude/rules/testing/test-anti-patterns.md +++ b/.claude/rules/testing/test-anti-patterns.md @@ -17,3 +17,9 @@ paths: Never create `Project.toml`, `.venv/`, or `renv.lock` in test fixture directories. **Details:** `llm-docs/testing-patterns.md` → "Shared Test Environments" + +## Don't: `Deno.chdir()` inside the test body + +`Deno.chdir()` mutates process-global cwd, so a test that changes it can leak into other tests in the same process. The harness already changes and restores the working directory: return the directory from `TestContext.cwd`, create fixtures in `setup`, clean up in `teardown` (examples: `tests/unit/dotenv-config.test.ts`, `tests/smoke/use/template.test.ts`). For a temp directory you don't need to run *from*, use `withTempDir` (`tests/utils.ts`). A test that only needs a *relative* input can pass a path relative to the current cwd without changing it. + +**Details:** `llm-docs/testing-patterns.md` → "Working-Directory-Sensitive Tests" diff --git a/llm-docs/testing-patterns.md b/llm-docs/testing-patterns.md index fd36ab1ff6e..f18754caf43 100644 --- a/llm-docs/testing-patterns.md +++ b/llm-docs/testing-patterns.md @@ -169,6 +169,40 @@ testQuartoCmd( - Clean up entire temp directory including source files - File verification uses relative paths when checking files in `cwd()` +### Working-Directory-Sensitive Tests + +Some tests need to run from a specific directory (e.g. reproducing a bug that +depends on the process cwd). **Do not `Deno.chdir()` inside the test body** — +it mutates process-global cwd and can leak into other tests in the same +process. Use the `TestContext` options instead; the harness changes the cwd +before the test and restores it afterward: + +```typescript +const workingDir = Deno.makeTempDirSync(); + +unitTest("runs from workingDir", async () => { + // cwd is workingDir here (set by the harness) +}, { + setup: () => { Deno.writeTextFileSync(".env.example", "..."); return Promise.resolve(); }, + cwd: () => workingDir, + teardown: () => { try { Deno.removeSync(workingDir, { recursive: true }); } catch { /* best-effort */ } return Promise.resolve(); }, +}); +``` + +**Key points:** + +- The harness calls `cwd()` **before** `setup()`, so the directory must already + exist when `cwd()` runs — create it at module scope, not in `setup`. +- `teardown` runs **before** the harness restores the cwd, so on Windows the + temp dir may still be the cwd and resist removal. Wrap the removal in + try/catch (best-effort) — see `tests/smoke/use/template.test.ts` and + `tests/unit/dotenv-config.test.ts`. +- For a temp directory you don't need to run *from*, prefer `withTempDir` + (`tests/utils.ts`), which creates and recursively removes it in a `finally`. +- A test that only needs a **relative** input (not a specific cwd) can pass a + path relative to the current cwd (`relative(Deno.cwd(), absFile)`) without + changing directories at all. + ## Verification Helpers ### Core Verifiers diff --git a/news/changelog-1.10.md b/news/changelog-1.10.md index c645725178f..3d2eda2e5ab 100644 --- a/news/changelog-1.10.md +++ b/news/changelog-1.10.md @@ -6,6 +6,7 @@ All changes included in 1.10: - ([#14281](https://github.com/quarto-dev/quarto-cli/issues/14281)): Fix transient `.quarto_ipynb` files accumulating during `quarto preview` with Jupyter engine. - ([#14298](https://github.com/quarto-dev/quarto-cli/issues/14298)): Fix `quarto preview` browse URL including output filename (e.g., `hello.html`) for single-file documents, breaking Posit Workbench proxied server access. - ([#14489](https://github.com/quarto-dev/quarto-cli/issues/14489)): Restore `--output-dir` support for `quarto preview` of single files when no `_quarto.yml` is present (e.g. R-package workspaces). Regression introduced in v1.9.18. +- ([#14683](https://github.com/quarto-dev/quarto-cli/issues/14683)): Fix `quarto preview` of a knitr document in a project subdirectory failing when run from that subdirectory with a bare filename (the shape RStudio's Render button uses). Regression introduced in v1.9.18. - ([rstudio/rstudio#17333](https://github.com/rstudio/rstudio/issues/17333)): Fix `quarto inspect` on standalone files emitting project metadata that breaks RStudio's publishing wizard. ## Dependencies @@ -108,6 +109,7 @@ All changes included in 1.10: - ([#6092](https://github.com/quarto-dev/quarto-cli/issues/6092)): Fix the `default-image-extension` being appended to an extensionless URL ending in a slash (e.g. `![](https://example.com/)`), which broke the embed/iframe image syntax. - ([#6651](https://github.com/quarto-dev/quarto-cli/issues/6651)): Fix dart-sass compilation failing in enterprise environments where `.bat` files are blocked by group policy. - ([#11703](https://github.com/quarto-dev/quarto-cli/issues/11703)): Fix SCSS color-variable export (`--quarto-scss-export-*`) being silently skipped when a theme rule contains consecutive semicolons (e.g. `max-height: 100% !important;;`). +- ([#12401](https://github.com/quarto-dev/quarto-cli/issues/12401)): Fix `QUARTO_DOCUMENT_PATH` being a relative path for single-file renders, inconsistent with project renders where it was absolute. Execution target paths are now normalized to absolute at a single point, so single-file and project renders behave the same. - ([#14255](https://github.com/quarto-dev/quarto-cli/issues/14255)): Fix shortcodes inside inline and display math expressions not being resolved. - ([#14342](https://github.com/quarto-dev/quarto-cli/issues/14342)): Work around TOCTOU race in Deno's `expandGlobSync` that can cause unexpected exceptions to be raised while traversing directories during project initialization. - ([#14445](https://github.com/quarto-dev/quarto-cli/issues/14445)): Fix intermittent `Uncaught (in promise) TypeError: Writable stream is closed or errored.` aborting renders on Linux. `execProcess` now awaits and swallows the rejection from `process.stdin.close()` when the child closes its stdin first. The captured stderr is now also surfaced when `typst-gather analyze` falls back to staging all packages, so failures are diagnosable without bypassing `quarto`. diff --git a/src/execute/engine.ts b/src/execute/engine.ts index 0afab0a88f2..18a554921f7 100644 --- a/src/execute/engine.ts +++ b/src/execute/engine.ts @@ -12,7 +12,7 @@ import { partitionYamlFrontMatter, readYamlFromMarkdown, } from "../core/yaml.ts"; -import { dirAndStem } from "../core/path.ts"; +import { dirAndStem, normalizePath } from "../core/path.ts"; import { metadataAsFormat } from "../config/metadata.ts"; import { kBaseFormat, kEngine } from "../config/constants.ts"; @@ -355,6 +355,19 @@ export async function fileExecutionEngineAndTarget( flags: RenderFlags | undefined, project: ProjectContext, ): Promise<{ engine: ExecutionEngineInstance; target: ExecutionTarget }> { + // Establish an "always absolute" contract for the file path at this single + // convergence point for all callers (preview format-resolution, render, and + // project render). fileInformationCache keys are normalized to absolute + // (FileInformationCacheMap), so a caller passing a cwd-relative path — e.g. + // preview seeding the cache with the bare filename RStudio's Render button + // uses from a subdirectory — would otherwise store a target whose + // source/input stay relative under an absolute key, a stale value that a + // later absolute-path lookup reuses. Normalizing here keeps target.source and + // target.input consistent with the key and with project renders (which + // already pass absolute paths). See #14151 (the contract), #12401 + // (QUARTO_DOCUMENT_PATH / cwd inconsistency), #14150 (doubled subdirectory), + // and #14683 (knitr abs_path failure previewing from a subdirectory). + file = normalizePath(file); const cached = ensureFileInformationCache(project, file); if (cached && cached.engine && cached.target) { return { engine: cached.engine, target: cached.target }; diff --git a/tests/unit/preview-subdir-knitr-cwd.test.ts b/tests/unit/preview-subdir-knitr-cwd.test.ts new file mode 100644 index 00000000000..43035175b9a --- /dev/null +++ b/tests/unit/preview-subdir-knitr-cwd.test.ts @@ -0,0 +1,245 @@ +/* + * preview-subdir-knitr-cwd.test.ts + * + * Regression test for quarto-dev/quarto-cli#14683. + * + * When previewing a knitr-engine document that lives in a project + * subdirectory, with the working directory set to that subdirectory and the + * input passed as a bare filename (the shape RStudio's Render button uses), + * the render failed with: + * + * Error in rmarkdown:::abs_path(input) : + * The file 'claudetest.qmd' does not exist. + * + * Root cause: preview resolves the preview format first (renderFormats / + * previewFormat) using the cwd-relative filename, which seeds the shared + * ProjectContext's fileInformationCache with an ExecutionTarget whose `input` + * is that relative string. The cache is keyed by normalized (absolute) path, so + * the later renderProject() call — which passes the absolute path — hits the + * stale cache entry and inherits the relative `input`. The knitr subprocess + * runs with cwd = project dir, where the subdirectory-relative filename does + * not resolve, so abs_path() throws. + * + * fileExecutionEngineAndTarget now normalizes the path to absolute at that + * single convergence point, so the cached target is absolute regardless of how + * the caller spelled the path. + * + * Copyright (C) 2026 Posit Software, PBC + */ + +import { unitTest } from "../test.ts"; +import { assert } from "testing/asserts"; +import { dirname, isAbsolute, join, relative } from "../../src/deno_ral/path.ts"; +import { existsSync } from "../../src/deno_ral/fs.ts"; +import { which } from "../../src/core/path.ts"; +import { withTempDir } from "../utils.ts"; +import { projectContext } from "../../src/project/project-context.ts"; +import { singleFileProjectContext } from "../../src/project/types/single-file/single-file.ts"; +import { notebookContext } from "../../src/render/notebook/notebook-context.ts"; +import { renderServices } from "../../src/command/render/render-services.ts"; +import { renderFormats } from "../../src/command/render/render-contexts.ts"; +import { previewFormat } from "../../src/command/preview/preview.ts"; +import { renderProject } from "../../src/command/render/project.ts"; +import { initYamlIntelligenceResourcesFromFilesystem } from "../../src/core/schema/utils.ts"; + +// --- End-to-end fixture (RED-1) --- +// The harness applies TestContext.cwd() BEFORE setup(), so the working +// directory must already exist when it chdirs in — create the tree at module +// scope. This test keeps the literal RStudio shape: cwd = the doc's +// subdirectory, input = the bare filename. +const e2eBase = Deno.makeTempDirSync({ prefix: "quarto_test_14683_" }); +const e2eProjDir = join(e2eBase, "testsite"); +const e2eLabsDir = join(e2eProjDir, "labs"); +Deno.mkdirSync(e2eLabsDir, { recursive: true }); + +unitTest( + "preview of a knitr subdir doc from that subdir's cwd renders (#14683)", + async () => { + await initYamlIntelligenceResourcesFromFilesystem(); + + // cwd is e2eLabsDir (set by the harness via TestContext.cwd). + const file = "claudetest.qmd"; + + const nbContext = notebookContext(); + const project = (await projectContext(dirname(file), nbContext)) ?? + (await singleFileProjectContext(file, nbContext)); + + try { + // Mimic preview cmd.ts: resolve the preview format first, using the + // bare filename. This seeds the shared context's fileInformationCache. + const services = renderServices(nbContext); + try { + const formats = await renderFormats(file, services, "all", project); + await previewFormat(file, project, undefined, formats); + } finally { + services.cleanup(); + } + + // Now render as preview does, passing the bare filename. + const renderServices2 = renderServices(nbContext); + try { + const result = await renderProject( + project, + { + services: renderServices2, + progress: false, + useFreezer: false, + flags: { to: "html" }, + pandocArgs: [], + previewServer: true, + }, + [file], + ); + + // The behavioral signal for this bug is whether the knitr render + // actually produced output. With the bug, the R subprocess fails at + // rmarkdown:::abs_path(input) (execute.R) before Pandoc runs, so no + // HTML is written. The wrapped renderProject error carries no message + // (render-files.ts), so we assert on the output itself, not error text. + const outputHtml = join(e2eProjDir, "_site", "labs", "claudetest.html"); + assert( + existsSync(outputHtml), + "Expected rendered HTML at _site/labs/claudetest.html — knitr render " + + "of the subdir doc failed to produce output (regression #14683)", + ); + + // Mere existence is not enough: prove the knitr chunk actually + // executed. `1 + 1` yields `[1] 2` in the rendered output, confirming + // the R engine ran end-to-end (the exact path the bug broke). + const html = Deno.readTextFileSync(outputHtml); + assert( + /\[1\]\s*2/.test(html), + "Rendered HTML is missing the knitr chunk result ([1] 2) — the R " + + "engine did not execute the code cell (regression #14683)", + ); + + // QUARTO_DOCUMENT_PATH (from target.source) must point at the doc's + // subdirectory (…/testsite/labs), not "." or the project root. A + // relative source would make it wrong here (#12401). + assert( + /testsite[\\/]labs/.test(html), + "QUARTO_DOCUMENT_PATH did not resolve to the doc's subdirectory " + + "(expected a path containing testsite/labs) — regression #14683 / #12401", + ); + + assert( + !result.error, + `renderProject reported an error: ${result.error?.message}`, + ); + } finally { + renderServices2.cleanup(); + } + } finally { + project.cleanup(); + } + }, + { + // Spawns the R/knitr engine; skip (not fail) where R is absent. + prereq: async () => (await which("Rscript")) !== undefined, + // Run from the doc's subdirectory so the bare filename is cwd-relative, + // exactly as RStudio's Render button invokes preview. The harness restores + // the original cwd afterward. + cwd: () => e2eLabsDir, + setup: () => { + Deno.writeTextFileSync( + join(e2eProjDir, "_quarto.yml"), + 'project:\n type: website\n\nwebsite:\n title: "testsite"\n', + ); + Deno.writeTextFileSync( + join(e2eProjDir, "index.qmd"), + "---\ntitle: Index\n---\n\nHome.\n", + ); + // Second chunk echoes QUARTO_DOCUMENT_PATH so the render also proves the + // env var (derived from target.source) resolves to the doc's subdirectory + // rather than "." / the project root — the #12401 inconsistency. + Deno.writeTextFileSync( + join(e2eLabsDir, "claudetest.qmd"), + "---\ntitle: Claude Test\n---\n\n```{r}\n1 + 1\n```\n\n" + + '```{r}\ncat("QDP:", Sys.getenv("QUARTO_DOCUMENT_PATH"))\n```\n', + ); + return Promise.resolve(); + }, + teardown: () => { + // Best-effort: the harness restores cwd after teardown, so on Windows the + // dir may still be the cwd and resist removal — acceptable, matching the + // house pattern (see tests/smoke/use/template.test.ts). + try { + Deno.removeSync(e2eBase, { recursive: true }); + } catch { + // ignore + } + return Promise.resolve(); + }, + }, +); + +// Deterministic, R-free guard for the root cause. Seeding the cache with a +// relative path (as preview's format resolution does) and then looking up by +// the absolute path (as renderProject does) must return one cached target whose +// input/source are absolute — never the stale relative string. This is what the +// knitr R subprocesses (execute, dependencies, postprocess) read, so asserting +// the target is absolute covers every reader without needing R. A fix that only +// absolutized at execute time would leave the cached target relative and fail. +// No cwd change needed: a path relative to the current cwd exercises the same +// normalization the bug's bare filename would. +unitTest( + "fileExecutionEngineAndTarget yields an absolute target for a relative subdir input (#14683)", + async () => { + await initYamlIntelligenceResourcesFromFilesystem(); + + await withTempDir(async (tempBase) => { + const projDir = join(tempBase, "testsite"); + const labsDir = join(projDir, "labs"); + Deno.mkdirSync(labsDir, { recursive: true }); + + Deno.writeTextFileSync( + join(projDir, "_quarto.yml"), + 'project:\n type: website\n\nwebsite:\n title: "testsite"\n', + ); + Deno.writeTextFileSync( + join(labsDir, "claudetest.qmd"), + "---\ntitle: Claude Test\n---\n\n```{r}\n1 + 1\n```\n", + ); + + const absFile = join(labsDir, "claudetest.qmd"); + // A cwd-relative path — the "relative input" the preview seed carries. + const relFile = relative(Deno.cwd(), absFile); + + const nbContext = notebookContext(); + const project = (await projectContext(labsDir, nbContext)) ?? + (await singleFileProjectContext(absFile, nbContext)); + + try { + // Seed with the relative path. Building the target reads only + // markdown/YAML (no R execution), so this is deterministic. + const seeded = await project.fileExecutionEngineAndTarget(relFile); + assert( + isAbsolute(seeded.target.input) && isAbsolute(seeded.target.source), + `Seeded target should be absolute; got input=${seeded.target.input} ` + + `source=${seeded.target.source} (#14683)`, + ); + + // Look up by the absolute path (renderProject's spelling). The + // normalized cache key collides with the seeded entry; the returned + // target must still be absolute and be the same cached object. + const resolved = await project.fileExecutionEngineAndTarget(absFile); + assert( + isAbsolute(resolved.target.input), + `Expected absolute target.input after absolute lookup, got ` + + `${resolved.target.input} (#14683)`, + ); + assert( + isAbsolute(resolved.target.source), + `Expected absolute target.source after absolute lookup, got ` + + `${resolved.target.source} (#14683)`, + ); + assert( + resolved.target === seeded.target, + "Relative and absolute lookups must hit the same cached target (#14683)", + ); + } finally { + project.cleanup(); + } + }, "quarto_test_14683b_"); + }, +); diff --git a/tests/unit/single-file-input-abs-12401.test.ts b/tests/unit/single-file-input-abs-12401.test.ts new file mode 100644 index 00000000000..50d472bb722 --- /dev/null +++ b/tests/unit/single-file-input-abs-12401.test.ts @@ -0,0 +1,69 @@ +/* + * single-file-input-abs-12401.test.ts + * + * Regression test for quarto-dev/quarto-cli#12401. + * + * Single-file (non-project) rendering used to build an ExecutionTarget whose + * `source`/`input` were the caller's relative path, while project rendering + * normalized to an absolute path (via project.ts normalizeFiles). That + * inconsistency leaked into `target.source`-derived values — notably + * QUARTO_DOCUMENT_PATH (src/execute/environment.ts sets it to + * dirname(target.source)) — which was relative for single files but absolute + * inside a project. + * + * fileExecutionEngineAndTarget now normalizes the path to absolute at that + * single convergence point, so single-file and project renders agree: the + * target is always absolute. + * + * Deterministic and engine-free: building the target only reads markdown/YAML. + * + * Copyright (C) 2026 Posit Software, PBC + */ + +import { unitTest } from "../test.ts"; +import { assert } from "testing/asserts"; +import { isAbsolute, join, relative } from "../../src/deno_ral/path.ts"; +import { withTempDir } from "../utils.ts"; +import { singleFileProjectContext } from "../../src/project/types/single-file/single-file.ts"; +import { notebookContext } from "../../src/render/notebook/notebook-context.ts"; +import { initYamlIntelligenceResourcesFromFilesystem } from "../../src/core/schema/utils.ts"; + +unitTest( + "single-file render builds an absolute target from a relative arg (#12401)", + async () => { + await initYamlIntelligenceResourcesFromFilesystem(); + + await withTempDir(async (tempBase) => { + const absFile = join(tempBase, "doc.qmd"); + Deno.writeTextFileSync(absFile, "---\ntitle: Doc\n---\n\nHello.\n"); + + // A cwd-relative path — the single-file (non-project) render path used to + // keep this verbatim in target.source/input, diverging from project + // renders (which are absolute). + const relFile = relative(Deno.cwd(), absFile); + + const nbContext = notebookContext(); + const project = await singleFileProjectContext(relFile, nbContext); + + try { + const { target } = await project.fileExecutionEngineAndTarget(relFile); + + // Before the fix, target.source/input were the relative arg, so + // QUARTO_DOCUMENT_PATH (dirname(target.source)) diverged from project + // renders. They are now absolute in both cases. + assert( + isAbsolute(target.source), + `Expected an absolute target.source for single-file render, got ` + + `${target.source} (regression #12401)`, + ); + assert( + isAbsolute(target.input), + `Expected an absolute target.input for single-file render, got ` + + `${target.input} (regression #12401)`, + ); + } finally { + project.cleanup(); + } + }, "quarto_test_12401_"); + }, +); From f810757394424fb1f4960769f40122001553cea4 Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Wed, 15 Jul 2026 18:40:25 +0200 Subject: [PATCH 2/9] Add manual preview regression check for knitr subdirectory preview (#14683) An automated `quarto preview` test is not practical (preview starts a blocking server), so document the real-CLI reproduction as a manual test-matrix entry: previewing a knitr document from its own subdirectory with a bare filename (the RStudio Render-button shape) must render without the rmarkdown:::abs_path failure, and QUARTO_DOCUMENT_PATH must resolve to the doc's subdirectory. --- tests/docs/manual/preview/README.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/docs/manual/preview/README.md b/tests/docs/manual/preview/README.md index 36b0c7d8566..55bf7f628e8 100644 --- a/tests/docs/manual/preview/README.md +++ b/tests/docs/manual/preview/README.md @@ -161,6 +161,13 @@ After every change to preview URL or handler logic, verify that single-file prev - **Expected:** HTTP 200. Browse URL may include path for non-index files in project context. - **Catches:** `isSingleFile` guard accidentally excluding real project files from path computation +#### T33: knitr doc in a subdirectory — preview from that subdirectory (#14683) + +- **Setup:** Website project with `_quarto.yml`; a knitr doc at `labs/doc.qmd` containing an `{r}` chunk (e.g. `1 + 1`). Requires R + `rmarkdown`/`knitr`. +- **Steps:** From `labs/` (the doc's own directory), run `quarto preview doc.qmd --to html --no-watch-inputs --no-browser --port XXXX` — the shape RStudio's Render button uses (bare filename, cwd = the doc's subdirectory). +- **Expected:** Renders without `Error in rmarkdown:::abs_path(input)`; `_site/labs/doc.html` is produced with the executed chunk output. `QUARTO_DOCUMENT_PATH` resolves to the doc's absolute subdirectory (not `.` / the project root). +- **Catches:** `fileExecutionEngineAndTarget` not normalizing the input to absolute — a cwd-relative `target.input`/`source` in the shared preview cache makes the knitr R subprocess (cwd = project dir) fail `abs_path` on the subdirectory-relative filename. + ## Test Matrix: Format Change Detection (#14533) After every change to `previewRenderRequestIsCompatible` or the From 561a43f196dfcf484c13cb71654f390d768c0bf6 Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Wed, 15 Jul 2026 18:45:28 +0200 Subject: [PATCH 3/9] Add on-disk fixture for the #14683 manual preview check (T33) The T33 matrix entry described its setup inline but shipped no fixture, so the test was not self-contained. Add knitr-subdir-14683/ (a website project with a knitr doc at labs/doc.qmd that echoes QUARTO_DOCUMENT_PATH) and point T33 at it, so a tester can run the real-CLI reproduction directly. --- tests/docs/manual/preview/README.md | 6 +++--- .../preview/knitr-subdir-14683/.gitignore | 4 ++++ .../preview/knitr-subdir-14683/_quarto.yml | 5 +++++ .../preview/knitr-subdir-14683/index.qmd | 6 ++++++ .../preview/knitr-subdir-14683/labs/doc.qmd | 20 +++++++++++++++++++ 5 files changed, 38 insertions(+), 3 deletions(-) create mode 100644 tests/docs/manual/preview/knitr-subdir-14683/.gitignore create mode 100644 tests/docs/manual/preview/knitr-subdir-14683/_quarto.yml create mode 100644 tests/docs/manual/preview/knitr-subdir-14683/index.qmd create mode 100644 tests/docs/manual/preview/knitr-subdir-14683/labs/doc.qmd diff --git a/tests/docs/manual/preview/README.md b/tests/docs/manual/preview/README.md index 55bf7f628e8..aa17e01dd25 100644 --- a/tests/docs/manual/preview/README.md +++ b/tests/docs/manual/preview/README.md @@ -163,9 +163,9 @@ After every change to preview URL or handler logic, verify that single-file prev #### T33: knitr doc in a subdirectory — preview from that subdirectory (#14683) -- **Setup:** Website project with `_quarto.yml`; a knitr doc at `labs/doc.qmd` containing an `{r}` chunk (e.g. `1 + 1`). Requires R + `rmarkdown`/`knitr`. -- **Steps:** From `labs/` (the doc's own directory), run `quarto preview doc.qmd --to html --no-watch-inputs --no-browser --port XXXX` — the shape RStudio's Render button uses (bare filename, cwd = the doc's subdirectory). -- **Expected:** Renders without `Error in rmarkdown:::abs_path(input)`; `_site/labs/doc.html` is produced with the executed chunk output. `QUARTO_DOCUMENT_PATH` resolves to the doc's absolute subdirectory (not `.` / the project root). +- **Setup:** Fixture `knitr-subdir-14683/` — a website project with a knitr doc at `labs/doc.qmd` (an `{r}` chunk plus a `QUARTO_DOCUMENT_PATH` echo). Requires R + `rmarkdown`/`knitr`. +- **Steps:** From `knitr-subdir-14683/labs/` (the doc's own directory), run `quarto preview doc.qmd --to html --no-watch-inputs --no-browser --port XXXX` — the shape RStudio's Render button uses (bare filename, cwd = the doc's subdirectory). +- **Expected:** Renders without `Error in rmarkdown:::abs_path(input)`; `_site/labs/doc.html` is produced showing `[1] 2` (the R chunk executed). The echoed `QUARTO_DOCUMENT_PATH` is the doc's absolute subdirectory (ends in `labs`), not `.` / the project root (#12401). - **Catches:** `fileExecutionEngineAndTarget` not normalizing the input to absolute — a cwd-relative `target.input`/`source` in the shared preview cache makes the knitr R subprocess (cwd = project dir) fail `abs_path` on the subdirectory-relative filename. ## Test Matrix: Format Change Detection (#14533) diff --git a/tests/docs/manual/preview/knitr-subdir-14683/.gitignore b/tests/docs/manual/preview/knitr-subdir-14683/.gitignore new file mode 100644 index 00000000000..00a02dceb55 --- /dev/null +++ b/tests/docs/manual/preview/knitr-subdir-14683/.gitignore @@ -0,0 +1,4 @@ +/.quarto/ +/_site/ +**/*_files/ +**/*.quarto_ipynb diff --git a/tests/docs/manual/preview/knitr-subdir-14683/_quarto.yml b/tests/docs/manual/preview/knitr-subdir-14683/_quarto.yml new file mode 100644 index 00000000000..3b71b759805 --- /dev/null +++ b/tests/docs/manual/preview/knitr-subdir-14683/_quarto.yml @@ -0,0 +1,5 @@ +project: + type: website + +website: + title: "Knitr Subdir Preview (#14683)" diff --git a/tests/docs/manual/preview/knitr-subdir-14683/index.qmd b/tests/docs/manual/preview/knitr-subdir-14683/index.qmd new file mode 100644 index 00000000000..81cd544d576 --- /dev/null +++ b/tests/docs/manual/preview/knitr-subdir-14683/index.qmd @@ -0,0 +1,6 @@ +--- +title: Home +--- + +Home page. The regression fixture is `labs/doc.qmd` — preview it from the +`labs/` directory with a bare filename (see T33). diff --git a/tests/docs/manual/preview/knitr-subdir-14683/labs/doc.qmd b/tests/docs/manual/preview/knitr-subdir-14683/labs/doc.qmd new file mode 100644 index 00000000000..c9149a48abb --- /dev/null +++ b/tests/docs/manual/preview/knitr-subdir-14683/labs/doc.qmd @@ -0,0 +1,20 @@ +--- +title: Claude Test +engine: knitr +--- + +A knitr document in a project subdirectory. Previewing it from this `labs/` +directory with a bare filename (`quarto preview doc.qmd`) is the shape +RStudio's Render button uses, and is what regressed in #14683. + +```{r} +1 + 1 +``` + +The rendered output must show `[1] 2` above (the R engine executed), and +`QUARTO_DOCUMENT_PATH` below must be this file's absolute directory (ending in +`labs`), not `.` or the project root (#12401). + +```{r} +cat("QUARTO_DOCUMENT_PATH:", Sys.getenv("QUARTO_DOCUMENT_PATH")) +``` From 65175392f3634458e9e5ae07462853ea1bd40b0a Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Thu, 16 Jul 2026 12:26:28 +0200 Subject: [PATCH 4/9] Harden #14683/#12401 regression tests: no leak on skip, no cross-drive false-pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The knitr e2e fixture was created at module scope (unconditional) but only cleaned up in teardown, which the harness skips along with cwd/setup whenever the Rscript prereq is false — leaking a temp dir on any run without R. Move creation into the prereq closure so it only happens when the test will actually run. Both the #14683 cache-invariant guard and the #12401 test derived their "relative" input via relative(Deno.cwd(), absFile). path.relative() falls back to returning the absolute path unchanged when its arguments are on different Windows drives, which would let these tests pass even without the fix on a machine/CI job where the OS temp drive differs from the checkout drive (see test-smokes.yml's existing "Fix temp dir to use runner one (windows)" step, added for exactly this drive-mismatch reason). Switch both to TestContext.cwd with a bare filename, removing the dependency on Deno.cwd()'s relationship to the temp directory's drive entirely. --- tests/unit/preview-subdir-knitr-cwd.test.ts | 143 +++++++++++------- .../unit/single-file-input-abs-12401.test.ts | 80 ++++++---- 2 files changed, 135 insertions(+), 88 deletions(-) diff --git a/tests/unit/preview-subdir-knitr-cwd.test.ts b/tests/unit/preview-subdir-knitr-cwd.test.ts index 43035175b9a..d52a3e2f39e 100644 --- a/tests/unit/preview-subdir-knitr-cwd.test.ts +++ b/tests/unit/preview-subdir-knitr-cwd.test.ts @@ -29,10 +29,9 @@ import { unitTest } from "../test.ts"; import { assert } from "testing/asserts"; -import { dirname, isAbsolute, join, relative } from "../../src/deno_ral/path.ts"; +import { dirname, isAbsolute, join } from "../../src/deno_ral/path.ts"; import { existsSync } from "../../src/deno_ral/fs.ts"; import { which } from "../../src/core/path.ts"; -import { withTempDir } from "../utils.ts"; import { projectContext } from "../../src/project/project-context.ts"; import { singleFileProjectContext } from "../../src/project/types/single-file/single-file.ts"; import { notebookContext } from "../../src/render/notebook/notebook-context.ts"; @@ -44,13 +43,15 @@ import { initYamlIntelligenceResourcesFromFilesystem } from "../../src/core/sche // --- End-to-end fixture (RED-1) --- // The harness applies TestContext.cwd() BEFORE setup(), so the working -// directory must already exist when it chdirs in — create the tree at module -// scope. This test keeps the literal RStudio shape: cwd = the doc's -// subdirectory, input = the bare filename. -const e2eBase = Deno.makeTempDirSync({ prefix: "quarto_test_14683_" }); -const e2eProjDir = join(e2eBase, "testsite"); -const e2eLabsDir = join(e2eProjDir, "labs"); -Deno.mkdirSync(e2eLabsDir, { recursive: true }); +// directory must already exist when it chdirs in — create the tree in the +// prereq check itself (the harness runs prereq before cwd()/setup(), and +// skips cwd()/setup()/teardown() together when prereq is false), so nothing +// is left on disk when Rscript is absent and the test is skipped. This test +// keeps the literal RStudio shape: cwd = the doc's subdirectory, input = the +// bare filename. +let e2eBase: string; +let e2eProjDir: string; +let e2eLabsDir: string; unitTest( "preview of a knitr subdir doc from that subdir's cwd renders (#14683)", @@ -134,8 +135,17 @@ unitTest( } }, { - // Spawns the R/knitr engine; skip (not fail) where R is absent. - prereq: async () => (await which("Rscript")) !== undefined, + // Spawns the R/knitr engine; skip (not fail) where R is absent. Creates + // the fixture tree as a side effect only when Rscript is present, so a + // skipped run leaves nothing on disk (teardown never runs otherwise). + prereq: async () => { + if ((await which("Rscript")) === undefined) return false; + e2eBase = Deno.makeTempDirSync({ prefix: "quarto_test_14683_" }); + e2eProjDir = join(e2eBase, "testsite"); + e2eLabsDir = join(e2eProjDir, "labs"); + Deno.mkdirSync(e2eLabsDir, { recursive: true }); + return true; + }, // Run from the doc's subdirectory so the bare filename is cwd-relative, // exactly as RStudio's Render button invokes preview. The harness restores // the original cwd afterward. @@ -180,66 +190,83 @@ unitTest( // knitr R subprocesses (execute, dependencies, postprocess) read, so asserting // the target is absolute covers every reader without needing R. A fix that only // absolutized at execute time would leave the cached target relative and fail. -// No cwd change needed: a path relative to the current cwd exercises the same -// normalization the bug's bare filename would. +// Uses TestContext.cwd + a bare filename (not relative(Deno.cwd(), absFile)): +// path.relative() falls back to returning the absolute path unchanged when its +// two arguments are on different Windows drives, which would make the "seed +// with a relative path" step silently seed with an absolute one instead and +// pass even without the fix (see .github/workflows/test-smokes.yml's "Fix temp +// dir to use runner one" step, which exists because the default drive for +// %TEMP% and the checkout can differ on GitHub's Windows runners). +const guardBase = Deno.makeTempDirSync({ prefix: "quarto_test_14683b_" }); +const guardProjDir = join(guardBase, "testsite"); +const guardLabsDir = join(guardProjDir, "labs"); +Deno.mkdirSync(guardLabsDir, { recursive: true }); + unitTest( "fileExecutionEngineAndTarget yields an absolute target for a relative subdir input (#14683)", async () => { await initYamlIntelligenceResourcesFromFilesystem(); - await withTempDir(async (tempBase) => { - const projDir = join(tempBase, "testsite"); - const labsDir = join(projDir, "labs"); - Deno.mkdirSync(labsDir, { recursive: true }); + // cwd is guardLabsDir (set by the harness via TestContext.cwd). + const absFile = join(guardLabsDir, "claudetest.qmd"); + const relFile = "claudetest.qmd"; + const nbContext = notebookContext(); + const project = (await projectContext(guardLabsDir, nbContext)) ?? + (await singleFileProjectContext(absFile, nbContext)); + + try { + // Seed with the relative path. Building the target reads only + // markdown/YAML (no R execution), so this is deterministic. + const seeded = await project.fileExecutionEngineAndTarget(relFile); + assert( + isAbsolute(seeded.target.input) && isAbsolute(seeded.target.source), + `Seeded target should be absolute; got input=${seeded.target.input} ` + + `source=${seeded.target.source} (#14683)`, + ); + + // Look up by the absolute path (renderProject's spelling). The + // normalized cache key collides with the seeded entry; the returned + // target must still be absolute and be the same cached object. + const resolved = await project.fileExecutionEngineAndTarget(absFile); + assert( + isAbsolute(resolved.target.input), + `Expected absolute target.input after absolute lookup, got ` + + `${resolved.target.input} (#14683)`, + ); + assert( + isAbsolute(resolved.target.source), + `Expected absolute target.source after absolute lookup, got ` + + `${resolved.target.source} (#14683)`, + ); + assert( + resolved.target === seeded.target, + "Relative and absolute lookups must hit the same cached target (#14683)", + ); + } finally { + project.cleanup(); + } + }, + { + cwd: () => guardLabsDir, + setup: () => { Deno.writeTextFileSync( - join(projDir, "_quarto.yml"), + join(guardProjDir, "_quarto.yml"), 'project:\n type: website\n\nwebsite:\n title: "testsite"\n', ); Deno.writeTextFileSync( - join(labsDir, "claudetest.qmd"), + join(guardLabsDir, "claudetest.qmd"), "---\ntitle: Claude Test\n---\n\n```{r}\n1 + 1\n```\n", ); - - const absFile = join(labsDir, "claudetest.qmd"); - // A cwd-relative path — the "relative input" the preview seed carries. - const relFile = relative(Deno.cwd(), absFile); - - const nbContext = notebookContext(); - const project = (await projectContext(labsDir, nbContext)) ?? - (await singleFileProjectContext(absFile, nbContext)); - + return Promise.resolve(); + }, + teardown: () => { try { - // Seed with the relative path. Building the target reads only - // markdown/YAML (no R execution), so this is deterministic. - const seeded = await project.fileExecutionEngineAndTarget(relFile); - assert( - isAbsolute(seeded.target.input) && isAbsolute(seeded.target.source), - `Seeded target should be absolute; got input=${seeded.target.input} ` + - `source=${seeded.target.source} (#14683)`, - ); - - // Look up by the absolute path (renderProject's spelling). The - // normalized cache key collides with the seeded entry; the returned - // target must still be absolute and be the same cached object. - const resolved = await project.fileExecutionEngineAndTarget(absFile); - assert( - isAbsolute(resolved.target.input), - `Expected absolute target.input after absolute lookup, got ` + - `${resolved.target.input} (#14683)`, - ); - assert( - isAbsolute(resolved.target.source), - `Expected absolute target.source after absolute lookup, got ` + - `${resolved.target.source} (#14683)`, - ); - assert( - resolved.target === seeded.target, - "Relative and absolute lookups must hit the same cached target (#14683)", - ); - } finally { - project.cleanup(); + Deno.removeSync(guardBase, { recursive: true }); + } catch { + // ignore — best-effort (see the anti-pattern doc's Windows cwd note) } - }, "quarto_test_14683b_"); + return Promise.resolve(); + }, }, ); diff --git a/tests/unit/single-file-input-abs-12401.test.ts b/tests/unit/single-file-input-abs-12401.test.ts index 50d472bb722..b6858f19dbc 100644 --- a/tests/unit/single-file-input-abs-12401.test.ts +++ b/tests/unit/single-file-input-abs-12401.test.ts @@ -22,48 +22,68 @@ import { unitTest } from "../test.ts"; import { assert } from "testing/asserts"; -import { isAbsolute, join, relative } from "../../src/deno_ral/path.ts"; -import { withTempDir } from "../utils.ts"; +import { isAbsolute, join } from "../../src/deno_ral/path.ts"; import { singleFileProjectContext } from "../../src/project/types/single-file/single-file.ts"; import { notebookContext } from "../../src/render/notebook/notebook-context.ts"; import { initYamlIntelligenceResourcesFromFilesystem } from "../../src/core/schema/utils.ts"; +// Uses TestContext.cwd + a bare filename rather than relative(Deno.cwd(), +// absFile): path.relative() falls back to returning the absolute path +// unchanged when its two arguments are on different Windows drives, which +// would make "the single-file render path used to keep this relative" +// silently pass even without the fix on a machine/CI job where the OS temp +// drive differs from the checkout drive. +const fixtureDir = Deno.makeTempDirSync({ prefix: "quarto_test_12401_" }); +const relFile = "doc.qmd"; +const absFile = join(fixtureDir, relFile); + unitTest( "single-file render builds an absolute target from a relative arg (#12401)", async () => { await initYamlIntelligenceResourcesFromFilesystem(); - await withTempDir(async (tempBase) => { - const absFile = join(tempBase, "doc.qmd"); - Deno.writeTextFileSync(absFile, "---\ntitle: Doc\n---\n\nHello.\n"); - - // A cwd-relative path — the single-file (non-project) render path used to - // keep this verbatim in target.source/input, diverging from project - // renders (which are absolute). - const relFile = relative(Deno.cwd(), absFile); + // cwd is fixtureDir (set by the harness via TestContext.cwd). + const nbContext = notebookContext(); + const project = await singleFileProjectContext(relFile, nbContext); - const nbContext = notebookContext(); - const project = await singleFileProjectContext(relFile, nbContext); + try { + const { target } = await project.fileExecutionEngineAndTarget(relFile); + // Before the fix, target.source/input were the relative arg, so + // QUARTO_DOCUMENT_PATH (dirname(target.source)) diverged from project + // renders. They are now absolute in both cases. + assert( + isAbsolute(target.source), + `Expected an absolute target.source for single-file render, got ` + + `${target.source} (regression #12401)`, + ); + assert( + isAbsolute(target.input), + `Expected an absolute target.input for single-file render, got ` + + `${target.input} (regression #12401)`, + ); + assert( + target.source === absFile, + `Expected target.source to resolve to ${absFile}, got ` + + `${target.source} (regression #12401)`, + ); + } finally { + project.cleanup(); + } + }, + { + cwd: () => fixtureDir, + setup: () => { + Deno.writeTextFileSync(absFile, "---\ntitle: Doc\n---\n\nHello.\n"); + return Promise.resolve(); + }, + teardown: () => { try { - const { target } = await project.fileExecutionEngineAndTarget(relFile); - - // Before the fix, target.source/input were the relative arg, so - // QUARTO_DOCUMENT_PATH (dirname(target.source)) diverged from project - // renders. They are now absolute in both cases. - assert( - isAbsolute(target.source), - `Expected an absolute target.source for single-file render, got ` + - `${target.source} (regression #12401)`, - ); - assert( - isAbsolute(target.input), - `Expected an absolute target.input for single-file render, got ` + - `${target.input} (regression #12401)`, - ); - } finally { - project.cleanup(); + Deno.removeSync(fixtureDir, { recursive: true }); + } catch { + // ignore — best-effort (see the anti-pattern doc's Windows cwd note) } - }, "quarto_test_12401_"); + return Promise.resolve(); + }, }, ); From 9e37e020f33034177fb4190c2619dae823a62a3f Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Fri, 17 Jul 2026 11:57:55 +0200 Subject: [PATCH 5/9] Surface renderProject error in the #14683 e2e regression test The missing-file assertion fired before the result.error check, so a CI-only failure gave no diagnostic beyond "no HTML produced". Include the error message/stack in the existence assertion so the next CI run shows the actual cause. --- tests/unit/preview-subdir-knitr-cwd.test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/unit/preview-subdir-knitr-cwd.test.ts b/tests/unit/preview-subdir-knitr-cwd.test.ts index d52a3e2f39e..7260af0d7db 100644 --- a/tests/unit/preview-subdir-knitr-cwd.test.ts +++ b/tests/unit/preview-subdir-knitr-cwd.test.ts @@ -95,13 +95,14 @@ unitTest( // The behavioral signal for this bug is whether the knitr render // actually produced output. With the bug, the R subprocess fails at // rmarkdown:::abs_path(input) (execute.R) before Pandoc runs, so no - // HTML is written. The wrapped renderProject error carries no message - // (render-files.ts), so we assert on the output itself, not error text. + // HTML is written. Surface renderProject's error (if any) first, since + // the missing-file assertion below would otherwise mask it. const outputHtml = join(e2eProjDir, "_site", "labs", "claudetest.html"); assert( existsSync(outputHtml), "Expected rendered HTML at _site/labs/claudetest.html — knitr render " + - "of the subdir doc failed to produce output (regression #14683)", + "of the subdir doc failed to produce output (regression #14683). " + + `renderProject error: ${result.error?.message} ${result.error?.stack}`, ); // Mere existence is not enough: prove the knitr chunk actually From e1190442a4c90e876024ba007d488574093b860d Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Fri, 17 Jul 2026 12:43:06 +0200 Subject: [PATCH 6/9] Add R package-availability probe to #14683 CI investigation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Probes whether Rscript can load rmarkdown/knitr from callR's actual spawn cwd (the project dir) before the real render runs, so a CI-only failure shows package/environment state instead of just "no HTML". Uses a script file rather than inline -e code: an -e argument with embedded quotes gets mis-tokenized by cmd.exe when Rscript resolves to a .bat shim (e.g. rig-managed R installs) — the same reason callR always passes a script file, never inline -e. --- tests/unit/preview-subdir-knitr-cwd.test.ts | 31 +++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/unit/preview-subdir-knitr-cwd.test.ts b/tests/unit/preview-subdir-knitr-cwd.test.ts index 7260af0d7db..47b0779a405 100644 --- a/tests/unit/preview-subdir-knitr-cwd.test.ts +++ b/tests/unit/preview-subdir-knitr-cwd.test.ts @@ -32,6 +32,7 @@ import { assert } from "testing/asserts"; import { dirname, isAbsolute, join } from "../../src/deno_ral/path.ts"; import { existsSync } from "../../src/deno_ral/fs.ts"; import { which } from "../../src/core/path.ts"; +import { rBinaryPath } from "../../src/core/resources.ts"; import { projectContext } from "../../src/project/project-context.ts"; import { singleFileProjectContext } from "../../src/project/types/single-file/single-file.ts"; import { notebookContext } from "../../src/render/notebook/notebook-context.ts"; @@ -66,6 +67,36 @@ unitTest( (await singleFileProjectContext(file, nbContext)); try { + // Temporary diagnostic (#14683 CI investigation): prove whether R can + // load rmarkdown/knitr from callR's actual spawn cwd (the project dir) + // on CI before blaming the render pipeline for the missing output. + { + // Pass a script file (not inline -e code) — an -e argument containing + // double quotes gets re-tokenized by cmd.exe when Rscript resolves to + // a .bat shim (e.g. rig-managed installs), corrupting the call. + // Production callR never uses -e for the same reason (rmd.R is a file). + const rscript = await rBinaryPath("Rscript"); + const probeScript = join(e2eBase, "probe.R"); + Deno.writeTextFileSync( + probeScript, + 'cat("LIBPATHS:", paste(.libPaths(), collapse="|"), "\\n")\n' + + "library(rmarkdown)\nlibrary(knitr)\ncat(\"PKG_OK\\n\")\n", + ); + const probe = new Deno.Command(rscript, { + args: [probeScript], + cwd: e2eProjDir, + stdout: "piped", + stderr: "piped", + }); + const probeResult = await probe.output(); + assert( + probeResult.success, + `R probe failed (code ${probeResult.code}) rscript=${rscript} in cwd=${e2eProjDir}\n` + + `STDOUT:\n${new TextDecoder().decode(probeResult.stdout)}\n` + + `STDERR:\n${new TextDecoder().decode(probeResult.stderr)}`, + ); + } + // Mimic preview cmd.ts: resolve the preview format first, using the // bare filename. This seeds the shared context's fileInformationCache. const services = renderServices(nbContext); From d0c3e16c60804d3bc6632c8e52bdec9354ca9fa4 Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Fri, 17 Jul 2026 13:12:52 +0200 Subject: [PATCH 7/9] Force renv activation in the #14683 e2e fixture to fix CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The diagnostic probe (previous commit) confirmed the CI failure: R's .Rprofile lookup is cwd-exact with no parent-directory search, and the regression's own repro shape requires the process cwd to be a scratch directory outside the repo (the doc's subdirectory). That cwd never finds tests/.Rprofile, so renv never activates, and on CI — where rmarkdown/knitr live only in tests/renv's project library — R fails with "there is no package called 'rmarkdown'" before ever reaching the #14683 code path. It never showed up locally because a typical dev R install has these packages in a library that's on .libPaths() regardless of renv activation. setup() now writes a .Rprofile into the fixture project directory that sets RENV_PROJECT and sources the real tests/renv/activate.R, so renv activates against the actual project/library regardless of the fixture's own cwd. Removes the now-unneeded diagnostic probe. Adds tests/unit/CLAUDE.md documenting the mechanism for future debugging, since the failure mode (fast, silent, error message swallowed by renderProject's error wrapping) is not obvious from the test output alone. --- tests/unit/CLAUDE.md | 79 +++++++++++++++++++++ tests/unit/preview-subdir-knitr-cwd.test.ts | 54 ++++++-------- 2 files changed, 101 insertions(+), 32 deletions(-) create mode 100644 tests/unit/CLAUDE.md diff --git a/tests/unit/CLAUDE.md b/tests/unit/CLAUDE.md new file mode 100644 index 00000000000..f70d62bebba --- /dev/null +++ b/tests/unit/CLAUDE.md @@ -0,0 +1,79 @@ +# tests/unit — notes for AI-assisted debugging + +## `preview-subdir-knitr-cwd.test.ts` — forced renv activation + +This test's first case (`preview of a knitr subdir doc from that subdir's cwd +renders (#14683)`) writes a `.Rprofile` into its scratch project directory +during `setup()`. If this test starts failing on CI with an R error like: + +``` +Error in library(rmarkdown) : there is no package called 'rmarkdown' +``` + +read this before touching product code (`src/execute/rmd.ts`, +`src/execute/engine.ts`) — it is very likely this workaround, not a +regression in Quarto itself. + +### Why it's there + +The test reproduces quarto-dev/quarto-cli#14683, which only manifests when +the process's working directory is the *document's subdirectory* (not the +project root, not `tests/`). The test harness enforces this with +`TestContext.cwd(() => e2eLabsDir)`, which does a real `Deno.chdir()` into a +scratch temp dir outside the repo checkout for the duration of the test. + +R's own `.Rprofile` lookup at startup is **cwd-exact** — it checks only the +process's own working directory, with no parent-directory search. Normal +smoke-all knitr renders never `chdir` away from `tests/` (they compute a +path *relative to* the current cwd instead — see `smoke-all.test.ts`), so +their R subprocess spawns with cwd = `tests/`, finds `tests/.Rprofile`, and +activates the `tests/renv` project automatically. This test's cwd is a +scratch dir with no `.Rprofile` of its own, so that activation never +happens. + +On CI, rmarkdown/knitr are installed **only** inside `tests/renv`'s project +library (see `tests/renv.lock`, `tests/configure-test-env.ps1|sh`) — not in +a global/system R library. Without renv activation, R falls back to +`R_LIBS_USER`/the base library, rmarkdown/knitr aren't there, and the +knitr subprocess fails before ever reaching the #14683 code path. The +failure is fast (well under a second) and mostly silent: `renderProject` +swallows the underlying rejection into a bare `Error` with no message +(see `src/command/render/render-files.ts`'s catch block), so the test's own +`result.error` diagnostic won't show the real cause either — only the raw +Rscript stderr does. + +This never showed up locally because a typical dev machine (e.g. an R +install managed by `rig`) has rmarkdown/knitr in a library that's on +`.libPaths()` regardless of renv activation. + +### The fix + +`setup()` writes a `.Rprofile` into the scratch project dir (`e2eProjDir` — +the directory `callR` actually spawns Rscript in for a non-single-file +project) containing: + +```r +Sys.setenv(RENV_PROJECT = "") +source("/renv/activate.R") +``` + +`tests/renv/activate.R` reads `RENV_PROJECT` to determine the project root +if it's set, falling back to `getwd()` otherwise (see the top of that file). +Setting it explicitly makes renv activate against the *real* `tests/renv` +project and library, regardless of the fixture's own cwd. The absolute path +is derived from `import.meta.url` (this test file's own location), not an +env var, so it's self-contained. + +### If this breaks again + +- Confirm the theory by reading the raw Rscript stderr in the CI log for + this test (search the job log for `preview-subdir-knitr-cwd`) — a missing + package error confirms renv never activated. +- Check whether `tests/renv/activate.R`'s `RENV_PROJECT` handling changed + (a renv version bump could alter this contract). +- Check whether `withinActiveRenv()` in `src/execute/rmd.ts` changed — it + decides the R subprocess's actual spawn cwd (`Deno.cwd()` vs + `projectDir`), which is where R looks for `.Rprofile` at startup. +- The second test in this file (`fileExecutionEngineAndTarget yields an + absolute target...`) is R-free and does not need this workaround — it + only exercises `fileExecutionEngineAndTarget`, never spawns Rscript. diff --git a/tests/unit/preview-subdir-knitr-cwd.test.ts b/tests/unit/preview-subdir-knitr-cwd.test.ts index 47b0779a405..e028ebca971 100644 --- a/tests/unit/preview-subdir-knitr-cwd.test.ts +++ b/tests/unit/preview-subdir-knitr-cwd.test.ts @@ -29,10 +29,14 @@ import { unitTest } from "../test.ts"; import { assert } from "testing/asserts"; -import { dirname, isAbsolute, join } from "../../src/deno_ral/path.ts"; +import { + dirname, + fromFileUrl, + isAbsolute, + join, +} from "../../src/deno_ral/path.ts"; import { existsSync } from "../../src/deno_ral/fs.ts"; import { which } from "../../src/core/path.ts"; -import { rBinaryPath } from "../../src/core/resources.ts"; import { projectContext } from "../../src/project/project-context.ts"; import { singleFileProjectContext } from "../../src/project/types/single-file/single-file.ts"; import { notebookContext } from "../../src/render/notebook/notebook-context.ts"; @@ -67,36 +71,6 @@ unitTest( (await singleFileProjectContext(file, nbContext)); try { - // Temporary diagnostic (#14683 CI investigation): prove whether R can - // load rmarkdown/knitr from callR's actual spawn cwd (the project dir) - // on CI before blaming the render pipeline for the missing output. - { - // Pass a script file (not inline -e code) — an -e argument containing - // double quotes gets re-tokenized by cmd.exe when Rscript resolves to - // a .bat shim (e.g. rig-managed installs), corrupting the call. - // Production callR never uses -e for the same reason (rmd.R is a file). - const rscript = await rBinaryPath("Rscript"); - const probeScript = join(e2eBase, "probe.R"); - Deno.writeTextFileSync( - probeScript, - 'cat("LIBPATHS:", paste(.libPaths(), collapse="|"), "\\n")\n' + - "library(rmarkdown)\nlibrary(knitr)\ncat(\"PKG_OK\\n\")\n", - ); - const probe = new Deno.Command(rscript, { - args: [probeScript], - cwd: e2eProjDir, - stdout: "piped", - stderr: "piped", - }); - const probeResult = await probe.output(); - assert( - probeResult.success, - `R probe failed (code ${probeResult.code}) rscript=${rscript} in cwd=${e2eProjDir}\n` + - `STDOUT:\n${new TextDecoder().decode(probeResult.stdout)}\n` + - `STDERR:\n${new TextDecoder().decode(probeResult.stderr)}`, - ); - } - // Mimic preview cmd.ts: resolve the preview format first, using the // bare filename. This seeds the shared context's fileInformationCache. const services = renderServices(nbContext); @@ -183,6 +157,22 @@ unitTest( // the original cwd afterward. cwd: () => e2eLabsDir, setup: () => { + // Force renv activation for the fixture project, regardless of cwd. + // R's own .Rprofile lookup is cwd-exact (no parent-directory search), + // and this fixture's cwd (e2eLabsDir, required to reproduce #14683) is + // outside tests/, so it would never find tests/.Rprofile on its own. + // Without this, CI environments where rmarkdown/knitr are installed + // only in tests/renv's project library (not a global R library) fail + // with "there is no package called 'rmarkdown'" before ever reaching + // the #14683 code path. See tests/unit/CLAUDE.md for the full story. + const testsDir = dirname(dirname(fromFileUrl(import.meta.url))) + .replaceAll("\\", "/"); + Deno.writeTextFileSync( + join(e2eProjDir, ".Rprofile"), + `Sys.setenv(RENV_PROJECT = "${testsDir}")\n` + + `source("${testsDir}/renv/activate.R")\n`, + ); + Deno.writeTextFileSync( join(e2eProjDir, "_quarto.yml"), 'project:\n type: website\n\nwebsite:\n title: "testsite"\n', From 0eb7093d851f0b9153e03df2e7764a912d9a2c8f Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Fri, 17 Jul 2026 13:19:06 +0200 Subject: [PATCH 8/9] Scope the #14683 renv-activation note to the one file that needs it tests/unit/CLAUDE.md would have loaded for every file touched under tests/unit/, but the renv-activation workaround only matters for preview-subdir-knitr-cwd.test.ts. Move it to .claude/rules/testing/ with paths frontmatter scoped to that single file, matching how the other testing rules (smoke-all-tests.md, playwright-tests.md) are scoped. --- .../rules/testing/unit-test-with-r.md | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) rename tests/unit/CLAUDE.md => .claude/rules/testing/unit-test-with-r.md (93%) diff --git a/tests/unit/CLAUDE.md b/.claude/rules/testing/unit-test-with-r.md similarity index 93% rename from tests/unit/CLAUDE.md rename to .claude/rules/testing/unit-test-with-r.md index f70d62bebba..3209811610b 100644 --- a/tests/unit/CLAUDE.md +++ b/.claude/rules/testing/unit-test-with-r.md @@ -1,8 +1,11 @@ -# tests/unit — notes for AI-assisted debugging +--- +paths: + - "tests/unit/preview-subdir-knitr-cwd.test.ts" +--- -## `preview-subdir-knitr-cwd.test.ts` — forced renv activation +# `preview-subdir-knitr-cwd.test.ts` — forced renv activation -This test's first case (`preview of a knitr subdir doc from that subdir's cwd +The first test case (`preview of a knitr subdir doc from that subdir's cwd renders (#14683)`) writes a `.Rprofile` into its scratch project directory during `setup()`. If this test starts failing on CI with an R error like: @@ -14,7 +17,7 @@ read this before touching product code (`src/execute/rmd.ts`, `src/execute/engine.ts`) — it is very likely this workaround, not a regression in Quarto itself. -### Why it's there +## Why it's there The test reproduces quarto-dev/quarto-cli#14683, which only manifests when the process's working directory is the *document's subdirectory* (not the @@ -46,7 +49,7 @@ This never showed up locally because a typical dev machine (e.g. an R install managed by `rig`) has rmarkdown/knitr in a library that's on `.libPaths()` regardless of renv activation. -### The fix +## The fix `setup()` writes a `.Rprofile` into the scratch project dir (`e2eProjDir` — the directory `callR` actually spawns Rscript in for a non-single-file @@ -64,7 +67,7 @@ project and library, regardless of the fixture's own cwd. The absolute path is derived from `import.meta.url` (this test file's own location), not an env var, so it's self-contained. -### If this breaks again +## If this breaks again - Confirm the theory by reading the raw Rscript stderr in the CI log for this test (search the job log for `preview-subdir-knitr-cwd`) — a missing From 8325ac0adcda4bdd4a340e592448e4f056174230 Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Fri, 17 Jul 2026 13:31:34 +0200 Subject: [PATCH 9/9] Split the R/renv-activation gotcha across rules, llm-docs, and code comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per repo convention, .claude/rules/testing/ files are terse pointers and llm-docs/testing-patterns.md holds the actual explanation — llm-docs explicitly excludes issue-specific notes. Move the detailed mechanism there as a generalized "R Tests That Change Working Directory" entry (no issue number, survives independently of #14683), trim the rule file back to a short pointer, and trim the test's own setup() comment to a one-line pointer plus what's specific to this fixture. --- .claude/rules/testing/unit-test-with-r.md | 83 ++------------------- llm-docs/testing-patterns.md | 30 ++++++++ tests/unit/preview-subdir-knitr-cwd.test.ts | 11 +-- 3 files changed, 41 insertions(+), 83 deletions(-) diff --git a/.claude/rules/testing/unit-test-with-r.md b/.claude/rules/testing/unit-test-with-r.md index 3209811610b..c826b0e544c 100644 --- a/.claude/rules/testing/unit-test-with-r.md +++ b/.claude/rules/testing/unit-test-with-r.md @@ -3,80 +3,13 @@ paths: - "tests/unit/preview-subdir-knitr-cwd.test.ts" --- -# `preview-subdir-knitr-cwd.test.ts` — forced renv activation +# R unit test that changes working directory -The first test case (`preview of a knitr subdir doc from that subdir's cwd -renders (#14683)`) writes a `.Rprofile` into its scratch project directory -during `setup()`. If this test starts failing on CI with an R error like: +This test runs a knitr subprocess with cwd set to a scratch directory +(required to reproduce the bug). R's `.Rprofile` lookup is cwd-exact, so it +won't pick up `tests/renv` activation there — on CI the R subprocess fails +with `there is no package called 'rmarkdown'`. `setup()` writes a +`.Rprofile` into the fixture dir to re-activate renv against the real +`tests/` project. -``` -Error in library(rmarkdown) : there is no package called 'rmarkdown' -``` - -read this before touching product code (`src/execute/rmd.ts`, -`src/execute/engine.ts`) — it is very likely this workaround, not a -regression in Quarto itself. - -## Why it's there - -The test reproduces quarto-dev/quarto-cli#14683, which only manifests when -the process's working directory is the *document's subdirectory* (not the -project root, not `tests/`). The test harness enforces this with -`TestContext.cwd(() => e2eLabsDir)`, which does a real `Deno.chdir()` into a -scratch temp dir outside the repo checkout for the duration of the test. - -R's own `.Rprofile` lookup at startup is **cwd-exact** — it checks only the -process's own working directory, with no parent-directory search. Normal -smoke-all knitr renders never `chdir` away from `tests/` (they compute a -path *relative to* the current cwd instead — see `smoke-all.test.ts`), so -their R subprocess spawns with cwd = `tests/`, finds `tests/.Rprofile`, and -activates the `tests/renv` project automatically. This test's cwd is a -scratch dir with no `.Rprofile` of its own, so that activation never -happens. - -On CI, rmarkdown/knitr are installed **only** inside `tests/renv`'s project -library (see `tests/renv.lock`, `tests/configure-test-env.ps1|sh`) — not in -a global/system R library. Without renv activation, R falls back to -`R_LIBS_USER`/the base library, rmarkdown/knitr aren't there, and the -knitr subprocess fails before ever reaching the #14683 code path. The -failure is fast (well under a second) and mostly silent: `renderProject` -swallows the underlying rejection into a bare `Error` with no message -(see `src/command/render/render-files.ts`'s catch block), so the test's own -`result.error` diagnostic won't show the real cause either — only the raw -Rscript stderr does. - -This never showed up locally because a typical dev machine (e.g. an R -install managed by `rig`) has rmarkdown/knitr in a library that's on -`.libPaths()` regardless of renv activation. - -## The fix - -`setup()` writes a `.Rprofile` into the scratch project dir (`e2eProjDir` — -the directory `callR` actually spawns Rscript in for a non-single-file -project) containing: - -```r -Sys.setenv(RENV_PROJECT = "") -source("/renv/activate.R") -``` - -`tests/renv/activate.R` reads `RENV_PROJECT` to determine the project root -if it's set, falling back to `getwd()` otherwise (see the top of that file). -Setting it explicitly makes renv activate against the *real* `tests/renv` -project and library, regardless of the fixture's own cwd. The absolute path -is derived from `import.meta.url` (this test file's own location), not an -env var, so it's self-contained. - -## If this breaks again - -- Confirm the theory by reading the raw Rscript stderr in the CI log for - this test (search the job log for `preview-subdir-knitr-cwd`) — a missing - package error confirms renv never activated. -- Check whether `tests/renv/activate.R`'s `RENV_PROJECT` handling changed - (a renv version bump could alter this contract). -- Check whether `withinActiveRenv()` in `src/execute/rmd.ts` changed — it - decides the R subprocess's actual spawn cwd (`Deno.cwd()` vs - `projectDir`), which is where R looks for `.Rprofile` at startup. -- The second test in this file (`fileExecutionEngineAndTarget yields an - absolute target...`) is R-free and does not need this workaround — it - only exercises `fileExecutionEngineAndTarget`, never spawns Rscript. +**Details:** `llm-docs/testing-patterns.md` → "R Tests That Change Working Directory" diff --git a/llm-docs/testing-patterns.md b/llm-docs/testing-patterns.md index f18754caf43..5dc9e9d2aae 100644 --- a/llm-docs/testing-patterns.md +++ b/llm-docs/testing-patterns.md @@ -397,6 +397,36 @@ Rscript -e "renv::install(); renv::snapshot()" **Note:** While Quarto supports local Project.toml files in document directories for production use, the quarto-cli test infrastructure specifically does NOT support this pattern. All test dependencies must be in the main `tests/` environment. +### R Tests That Change Working Directory + +R resolves `.Rprofile` from the **exact** process cwd (no parent-directory +search). On CI, rmarkdown/knitr live only in `tests/renv`'s project library, +activated when cwd is `tests/` (via `tests/.Rprofile` sourcing +`renv/activate.R`). Most knitr tests never leave `tests/` — they pass paths +relative to the current cwd instead of changing directories — so activation +happens automatically. + +A test that must run with cwd set elsewhere (a scratch temp dir, via +`TestContext.cwd()` — see "Working-Directory-Sensitive Tests" above) loses +that activation: the R subprocess starts outside `tests/`, renv never +activates, and package loads fail with `there is no package called +'rmarkdown'`. This is CI-only — a developer machine with rmarkdown on the +default `.libPaths()` masks it entirely. The render pipeline also tends to +swallow the underlying subprocess error, so the failure can be silent beyond +the bare package-load message. + +**Fix:** in the fixture's cwd, write a `.Rprofile` that re-points renv at the +real project, regardless of where the test's cwd actually is: + +```r +Sys.setenv(RENV_PROJECT = "") +source("/renv/activate.R") +``` + +`renv/activate.R` reads `RENV_PROJECT` to determine the project root if set, +falling back to `getwd()` otherwise — setting it explicitly decouples renv +activation from the test's cwd. + ## Best Practices 1. **Always clean up**: Use teardown to remove generated files diff --git a/tests/unit/preview-subdir-knitr-cwd.test.ts b/tests/unit/preview-subdir-knitr-cwd.test.ts index e028ebca971..cc249651c15 100644 --- a/tests/unit/preview-subdir-knitr-cwd.test.ts +++ b/tests/unit/preview-subdir-knitr-cwd.test.ts @@ -157,14 +157,9 @@ unitTest( // the original cwd afterward. cwd: () => e2eLabsDir, setup: () => { - // Force renv activation for the fixture project, regardless of cwd. - // R's own .Rprofile lookup is cwd-exact (no parent-directory search), - // and this fixture's cwd (e2eLabsDir, required to reproduce #14683) is - // outside tests/, so it would never find tests/.Rprofile on its own. - // Without this, CI environments where rmarkdown/knitr are installed - // only in tests/renv's project library (not a global R library) fail - // with "there is no package called 'rmarkdown'" before ever reaching - // the #14683 code path. See tests/unit/CLAUDE.md for the full story. + // Re-activate renv against the real tests/ project, regardless of + // this fixture's cwd — see llm-docs/testing-patterns.md → "R Tests + // That Change Working Directory" for why this is needed. const testsDir = dirname(dirname(fromFileUrl(import.meta.url))) .replaceAll("\\", "/"); Deno.writeTextFileSync(