Skip to content

test(node): unskip transaction rollback and add loadFile parity coverage#410

Merged
ospfranco merged 1 commit into
OP-Engineering:mainfrom
pbbadenhorst:fix/node-parity-rollback
May 21, 2026
Merged

test(node): unskip transaction rollback and add loadFile parity coverage#410
ospfranco merged 1 commit into
OP-Engineering:mainfrom
pbbadenhorst:fix/node-parity-rollback

Conversation

@pbbadenhorst
Copy link
Copy Markdown
Contributor

Summary

The Node test suite skipped the transaction rollback test with a comment
claiming transaction() "doesn't properly return a promise." That claim is
inaccurate — NodeDatabase.transaction() is async and already does
BEGIN / await fn() / COMMIT, with a catch that runs ROLLBACK and
rethrows. Unskipping the test as written passes; the skip comment was stale.

This closes the parity-coverage gaps in the Node test suite.

Changes

  • Unskip Transaction rollback and tighten it: assert the inner error is
    rethrown and that the transient row is absent after rollback — not just an
    unchanged row count, which could mask a partial commit.
  • Drop the stale // Skipped: ... comment from Transaction commit; the test
    was already running, only mislabelled.
  • Add a Load SQL file test exercising db.loadFile(). No test previously
    covered that path.

Test plan

  • cd node && npm test — 18/18 pass
  • cd node && npx tsc --noEmit — clean

@bernard-jigx
Copy link
Copy Markdown

bernard-jigx commented May 21, 2026

Small fix re-enabling tests and adding a load file test. This was picked up previously on another PR, but did not what to add it to that one as noise.

@ospfranco
Copy link
Copy Markdown
Contributor

sure, thanks!

@ospfranco ospfranco merged commit ab871de into OP-Engineering:main May 21, 2026
10 checks passed
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