Skip to content

test(integration): fix flaky JiT test racing on shared tmp dir cleanup - #2383

Closed
mbiernacik wants to merge 2 commits into
mainfrom
fix/jit-spec-resource-ownership
Closed

mbiernacik wants to merge 2 commits into
mainfrom
fix/jit-spec-resource-ownership

Conversation

@mbiernacik

Copy link
Copy Markdown
Contributor

The two tests in tests/integration/jit.spec.ts run in parallel and shared a TmpDirFixture cleaned up in afterEach, so the test that finished first deleted the other test's project directory. When that happened before the other test's JiT step, the JiT worker couldn't find @dataform/core and the run failed with expected 'FAILED' to deeply equal 'SUCCESSFUL'. BigQuery job history shows this in 16 of 388 CI runs over the last 90 days.

Changes:

  • Each test owns its project directory and removes it in finally, so tests can't delete each other's files.
  • Datasets get a per-run random suffix and are dropped when the test ends, so concurrent runs (e.g. two CI builds) can't interfere and nothing is left behind.
  • The status assertions include the action results, so a failure shows the underlying error.

@mbiernacik
mbiernacik requested a review from a team as a code owner October 15, 2026 12:09
Comment thread tests/integration/jit.spec.ts Outdated
Comment thread tests/integration/jit.spec.ts Outdated
Comment thread tests/integration/jit.spec.ts Outdated
Comment thread tests/integration/jit.spec.ts
Comment thread tests/integration/jit.spec.ts Outdated
name: string,
fn: (projectDir: string, datasetName: string) => Promise<void>,
) {
const projectDir = fs.mkdtempSync(path.join(process.env.TEST_TMPDIR, `${name}_`));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: process.env.TEST_TMPDIR is only set under Bazel. Outside it, path.join(undefined, ...) throws. Maybe process.env.TEST_TMPDIR || os.tmpdir()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, done.

@mbiernacik

Copy link
Copy Markdown
Contributor Author

Heads-up: we're going to close this PR in favour of #2385.

Both PRs fix the same root cause: integration tests reusing fixed BigQuery dataset names, so concurrent runs and tests drop or overwrite each other's data. This PR does it only for jit.spec.ts. #2385 does it across all the integration suites (jit, run, evaluate, bigquery, property_graph, plus the CLI e2e/JiT tests), with shared helpers in testing/bigquery.ts and a cleanup of stale datasets left behind by killed runs. Rather than having two different conventions (makeSuffix here vs makeUniqueSuffix there, which are also incompatible with the stale-cleanup regex), we're standardising on #2385.

The part that's unique to this PR, each JiT test owning and cleaning up its own temp directory instead of using TmpDirFixture(afterEach) in a parallel suite, plus the expectSuccessfulRun diagnostics, is being carried over to #2385 (see the review there). Nothing from this PR is lost. Further work continues on #2385.

@mbiernacik mbiernacik closed this Oct 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants