Skip to content

test: cover the implicit file context, isolate temp fixtures per test #1704

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mishushakov wants to merge 1 commit into from
Closed

test: cover the implicit file context, isolate temp fixtures per test #1704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions packages/js-sdk/tests/template/fileContext.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import os from 'node:os'
import { describe, expect, test } from 'vitest'
import { Template } from '../../src'

// Path to a committed, read-only fixture, relative to this file's directory.
// The implicit file context is the directory of the file that calls Template(),
// so it is always inside the repository — which is why the fixture is committed
// and never written to, rather than generated into a temp directory.
const fixturePath = 'fixtures/hello.txt'

describe('file context', () => {
test('defaults to the directory of the caller of Template()', async () => {
const implicit = Template().fromBaseImage().copy(fixturePath, 'hello.txt')
const explicit = Template({ fileContextPath: __dirname })
.fromBaseImage()
.copy(fixturePath, 'hello.txt')

// toJSON hashes each COPY's files, so the two serializations only match if
// the implicit context resolved to this file's directory, the glob found
// the fixture there, and its contents were read. This is the only test that
// exercises the implicit default end to end — every other template test

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is the one thing here I'd change, because it will outlive the PR description and it is what a future reader will trust when deciding whether the implicit default is covered.

"This is the only test that exercises the implicit default end to end — every other template test passes fileContextPath explicitly" is not quite right. stacktrace.test.ts builds a bare Template() and copies real sibling files through the implicit context:

const template = Template().fromBaseImage()
template.copy(['stacktrace.test.ts', 'tags.test.ts'], '.')

I confirmed it by mutation rather than by reading: applying the PR's own mutant (getCallerDirectory() dropped from the constructor) fails two pre-existing tests in that file — traces on step after multi-source copy and traces on step after copyItems — with No files found in stacktrace.test.ts.

That is still an argument for this test, just a different one. Those two catches are incidental, their failure surfaces inside a stack-trace assertion that points nowhere near the file context, and the two neighbouring cases (multiSourceCopySecondSource, copyItemsSecondItem) pass vacuously under the same mutant because they expect a copy-step failure anyway and cannot tell a mocked build error from a mis-resolved context. So the accurate claim is that this is the first test to assert the implicit context resolves to the caller's directory, rather than the only one to execute that path.

Worth also noting what the coverage delta actually shows: this test is the first caller of Template.toJSON in the whole js-sdk suite. The public static toJSON (89–94) and the private TemplateBase.toJSON (973–980) both had hit count 0 before this PR, and they are the entire js-sdk coverage gain here (+5 statements, +2 branch arms, +1 function). The implicit-context lines themselves were already at hit count 60 on base.

// passes fileContextPath explicitly, and the unit test for
// getCallerDirectory covers the helper in isolation, not its use here.
expect(await Template.toJSON(implicit)).toBe(
await Template.toJSON(explicit)
)
})

test('fails to resolve a source that is not in the context', async () => {
// Keeps the assertion above from passing vacuously: the hashes match
// because the fixture was found, not because a missing file hashes the
// same either way.
const wrongContext = Template({ fileContextPath: os.tmpdir() })

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use an isolated directory for the negative context

When another process or an earlier interrupted run leaves fixtures/hello.txt under the system temp directory, this context is no longer empty, so Template.toJSON succeeds and the test fails nondeterministically. Create a unique directory with mkdtemp and clean it up afterward, as the Python equivalent does, rather than inspecting the shared temp root.

Useful? React with 👍 / 👎.

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming Codex's P2 with a reproduction, because claude[bot]'s review dismissed it as ruled out.

The glob runs with cwd: os.tmpdir() and the pattern is the relative fixtures/hello.txt, so a fixtures/ subtree anywhere in the shared temp root resolves the source. With /tmp/fixtures/hello.txt present on this branch:

× fails to resolve a source that is not in the context
AssertionError: promise resolved "'{
  "steps": [..." instead of rejecting

The Python mirror stayed green through the same probe because it uses a fresh TemporaryDirectory(). Mirroring that here keeps the two sides equivalent and removes the shared-state dependency:

const emptyContext = await mkdtemp(join(tmpdir(), 'fileContext-test-'))
try {
  const wrongContext = Template({ fileContextPath: emptyContext })
    .fromBaseImage()
    .copy(fixturePath, 'hello.txt')

  await expect(Template.toJSON(wrongContext)).rejects.toThrow(/No files found/)
} finally {
  await rm(emptyContext, { recursive: true, force: true })
}

.fromBaseImage()
.copy(fixturePath, 'hello.txt')

await expect(Template.toJSON(wrongContext)).rejects.toThrow(
/No files found/
)
})
})

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional, and the natural place for it is this file rather than a follow-up: two arms of the very expression this file tests are still at zero.

index.ts:77  cond-expr   [0, 55]   // runtime === 'browser' ? '.' : ...
index.ts:77  binary-expr [55, 0]   // getCallerDirectory() ?? '.'

The second one I verified is genuinely reachable, and it has a user-visible consequence. captureUserFrames' doc comment names the case — "the SDK is bundled into the caller's file" — so I appended three lines to a copy of the built bundle:

$ cp packages/js-sdk/dist/index.mjs /tmp/scratch/bundled-app.mjs
$ printf '
const t = Template();
console.log(t.fileContextPath);
' >> /tmp/scratch/bundled-app.mjs
$ node /tmp/scratch/bundled-app.mjs
bundled caller -> fileContextPath = "."
cwd = /tmp/scratch

So a bundled consumer silently resolves copy() sources against process.cwd() instead of their own file's directory. tests/bundle/edgeCompat.test.ts already runs against dist/index.mjs, so that harness fits this assertion.

The browser arm is the cheaper one: no test in the repo constructs a Template under the browser or cloudflare projects, so index.ts:588 and :626 (the two "Browser runtime is not supported for copy" throws) are also dead, at if [0, 26] and [0, 4]. One browser test that constructs Template() and asserts copy() throws would close three branch arms and two statements, and #1699 just made that project cheap to run.

Don't mirror either of these into the Python file, though — see the note there.

3 changes: 3 additions & 0 deletions packages/js-sdk/tests/template/fixtures/hello.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
This file is the fixture for the implicit file-context test. It is read-only:
the implicit context is the directory of the file that calls Template(), which
is inside the repository, so nothing may be written here at test time.
14 changes: 5 additions & 9 deletions packages/js-sdk/tests/template/utils/getAllFilesInPath.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,20 @@
import { expect, test, describe, beforeAll, afterAll, beforeEach } from 'vitest'
import { expect, test, describe, afterEach, beforeEach } from 'vitest'
import { writeFile, mkdir, mkdtemp, rm } from 'fs/promises'
import { tmpdir } from 'os'
import { join, basename } from 'path'
import { getAllFilesInPath } from '../../../src/template/utils'

describe('getAllFilesInPath', () => {
// A temp directory, so a test run never writes into the repository tree.
// A fresh temp directory per test, so a run never writes into the repository
// tree and no fixture leaks from one test into the next.

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description's justification for this half checks out precisely, and since it is the whole reason for the change I measured it rather than trusting it. Under the default umask 022:

after mkdtemp (old beforeAll)      : 700
after rm + mkdir (old beforeEach)  : 755
after mkdtemp (new beforeEach)     : 700

So the old hook really did discard mkdtemp's 0700 on every test, exactly as claimed, and this restores it.

Small wording note on the comment itself: "no fixture leaks from one test into the next" was already true before, since the old beforeEach did rm then mkdir. The description is precise about this (it identifies the permissions and the uniqueness as what was lost, and calls the leak angle "not a bug today"); the comment is the looser of the two. The real gains are the 0700 mode above and no longer tearing down and recreating a fixed path — which is also the pattern most likely to be flaky on the Windows CI leg, where deleting and immediately recreating the same directory can hit EPERM/EBUSY.

No coverage or inventory movement from this half, as expected: all 17 tests here and all 7 in spoolTarArchive.test.ts ran and passed identically on base and head, with no status changes.

let testDir: string

beforeAll(async () => {
beforeEach(async () => {
testDir = await mkdtemp(join(tmpdir(), 'getAllFilesInPath-test-'))
})

afterAll(async () => {
await rm(testDir, { recursive: true, force: true })
})

beforeEach(async () => {
afterEach(async () => {
await rm(testDir, { recursive: true, force: true })
await mkdir(testDir, { recursive: true })
})

test('should return files matching a simple pattern', async () => {
Expand Down
14 changes: 5 additions & 9 deletions packages/js-sdk/tests/template/utils/spoolTarArchive.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { expect, test, describe, beforeAll, afterAll, beforeEach } from 'vitest'
import { expect, test, describe, afterEach, beforeEach } from 'vitest'
import {
writeFile,
mkdir,
Expand All @@ -15,20 +15,16 @@ import * as tar from 'tar'
import { ReadEntry } from 'tar'

describe('spoolTarArchive', () => {
// A temp directory, so a test run never writes into the repository tree.
// A fresh temp directory per test, so a run never writes into the repository
// tree and no fixture leaks from one test into the next.
let testDir: string

beforeAll(async () => {
beforeEach(async () => {
testDir = await mkdtemp(join(tmpdir(), 'spoolTarArchive-test-'))
})

afterAll(async () => {
await rm(testDir, { recursive: true, force: true })
})

beforeEach(async () => {
afterEach(async () => {
await rm(testDir, { recursive: true, force: true })
await mkdir(testDir, { recursive: true })
})

/**
Expand Down
3 changes: 3 additions & 0 deletions packages/python-sdk/tests/shared/template/fixtures/hello.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
This file is the fixture for the implicit file-context test. It is read-only:
the implicit context is the directory of the file that calls Template(), which
is inside the repository, so nothing may be written here at test time.
44 changes: 44 additions & 0 deletions packages/python-sdk/tests/shared/template/test_file_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import os
import tempfile

import pytest

from e2b import Template

# Path to a committed, read-only fixture, relative to this file's directory.
# The implicit file context is the directory of the file that calls Template(),
# so it is always inside the repository — which is why the fixture is committed
# and never written to, rather than generated into a temp directory.
FIXTURE_PATH = "fixtures/hello.txt"


def test_file_context_defaults_to_caller_directory():
implicit = Template().from_base_image().copy(FIXTURE_PATH, "hello.txt")
explicit = (
Template(file_context_path=os.path.dirname(__file__))
.from_base_image()
.copy(FIXTURE_PATH, "hello.txt")
)

# to_json hashes each COPY's files, so the two serializations only match if
# the implicit context resolved to this file's directory, the glob found the
# fixture there, and its contents were read. This is the only test that
# exercises the implicit default end to end — every other template test

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same wording point as the JS twin: test_stacktrace.py already reaches the implicit default. Under the PR's mutant (get_caller_directory() dropped from the constructor) four pre-existing tests fail — test_traces_on_step_after_multi_source_copy and test_traces_on_step_after_copy_items, in both the sync and async suites — while 152 of the 156 other offline template tests stay green. Their test_traces_on_second_source_of_multi_source_copy / ..._second_item_of_copy_items siblings pass vacuously under it. So this is the first assertion on the implicit default, not the first execution of it.

On the coverage side, this test contributes exactly one newly executed python line: e2b/template/main.py:1149, the body of to_json, which no test had ever called. Every constructor line was already executed on base.

One thing I'd explicitly not ask for here, since it looks like an obvious symmetry with the JS gap: the or "." fallback and utils.py lines 367 / 382 / 388–389 (branches (363, 367) and (381, 382)) are uncovered because they are effectively unreachable, not because they are untested. _is_user_file('<string>') returns True, so a synthesized frame still counts as user code:

>>> _is_user_file('<string>')
True
>>> exec(compile('from e2b import Template
ctx = Template()._file_context_path', '<string>', 'exec'), ns)
>>> ns['ctx']
'/workspace/packages/python-sdk'   # cwd, via the normal path — never the `or "."`

get_caller_frame() can only return None if every frame is inside the SDK package, which user code cannot arrange. Python has no analogue of the JS bundling case, because modules keep their filenames. Worth recording so the asymmetry isn't later read as a Python coverage hole.

# passes file_context_path explicitly, and the unit test for
# get_caller_directory covers the helper in isolation, not its use here.
assert Template.to_json(implicit) == Template.to_json(explicit)


def test_file_context_without_the_source_fails_to_resolve_it():
# Keeps the assertion above from passing vacuously: the hashes match because
# the fixture was found, not because a missing file hashes the same either
# way.
with tempfile.TemporaryDirectory() as empty_context:
wrong_context = (
Template(file_context_path=empty_context)
.from_base_image()
.copy(FIXTURE_PATH, "hello.txt")
)

with pytest.raises(ValueError, match="No files found"):

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking, and the cause is pre-existing. TASTE wants builder configuration failures to raise BuildException / BuildError rather than a bare ValueError / Error, and calculate_files_hash raises ValueError here (JS open-codes the same thing as new Error plus a manual error.stack, even though BuildError(message, stackTrace) exists for it). Inherited, not introduced — the builder has 6 bare raise ValueError sites in Python and 11 bare throw new Error sites in JS.

The reason to mention it on this line: the two new tests pin it asymmetrically. JS asserts only the message and would survive a fix; this one asserts the class, and BuildException extends Exception, not ValueError, so tightening the source breaks the Python mirror alone.

Template.to_json(wrong_context)
Loading