Skip to content

Commit 4e673fd

Browse files
committed
fix(sea): F4 fixup — skip-gate + drain-twice + drain-after-close edge tests
DA round 1 findings on F4 (fetchAll): H1 skip-gate (REPEAT of F2 H1): the F4 e2e imports `DBSQLClient` from `'../../../lib'`, which transitively pulls `SeaNativeLoader.ts`'s module-level `require('../../native/sea/index.js')`. When the `.node` artifact isn't built, that require throws MODULE_NOT_FOUND BEFORE mocha can invoke `before()`, crashing test discovery. Same root cause as F2 H1, repeating the fix: 1. Skip-gate verifies the native artifact exists in `before()`, skipping with a clear "run yarn build:native" message if absent 2. Switch the top-level `import { DBSQLClient }` to a `type`-only import (erased at compile time) + lazy `requireFromHere` inside the `connect()` helper via `createRequire(import.meta.url)` so the require works under both CJS and the ESM-reparse path mocha 11+ may use. M1 edge tests: add two cases covering drain-twice and drain-after-close, mirroring Thrift's `DBSQLOperation.fetchAll` behaviour: - drain-twice: after the first fetchAll drains the cursor to end-of-stream, a second fetchAll on the same operation returns an empty array (do/while body executes zero iterations, [] flattened). - drain-after-close: fetchAll on a closed operation throws (typed Error class). Doesn't pin the exact class because the SEA backend's closed-state error path may surface as either an `OperationStateError` or a kernel-envelope-decoded error depending on whether the close had reached the facade-level lifecycle flag or only the napi layer — both acceptable. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
1 parent 23df917 commit 4e673fd

1 file changed

Lines changed: 108 additions & 1 deletion

File tree

tests/e2e/sea/fetch-all-e2e.test.ts

Lines changed: 108 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,29 @@
3333
*/
3434

3535
import { expect } from 'chai';
36-
import { DBSQLClient } from '../../../lib';
36+
import * as fs from 'fs';
37+
import * as path from 'path';
38+
import { createRequire } from 'module';
3739
import type { ConnectionOptions } from '../../../lib/contracts/IDBSQLClient';
3840

41+
// Intentionally avoiding `import { DBSQLClient } from '../../../lib'`
42+
// at the top of the file. `DBSQLClient.ts` transitively imports
43+
// `SeaNativeLoader.ts`, which runs `const native =
44+
// require('../../native/sea/index.js')` at module-load time. If the
45+
// native `.node` artifact has not been built (`yarn build:native`),
46+
// that require throws `MODULE_NOT_FOUND` BEFORE mocha gets a chance
47+
// to invoke the `before()` skip-gate, crashing test discovery for the
48+
// whole suite. Lazy-require `DBSQLClient` inside the `connect()`
49+
// helper after the skip-gate has had a chance to fire. The `type`-only
50+
// import above is erased at compile time so it does not trigger any
51+
// runtime require. (DA round-1 H1 fixup; F2 same pattern.)
52+
//
53+
// `createRequire(import.meta.url)` so the require works under both
54+
// CJS and the ESM-reparse path mocha 11+ may use.
55+
56+
// eslint-disable-next-line @typescript-eslint/naming-convention
57+
const requireFromHere = createRequire(import.meta.url);
58+
3959
interface InternalConnectionOptionsAccess {
4060
useSEA?: boolean;
4161
}
@@ -51,10 +71,31 @@ describe('SEA fetchAll — Array<object> row drain', function suite() {
5171
if (!host || !path || !token) {
5272
// eslint-disable-next-line no-invalid-this
5373
this.skip();
74+
return;
75+
}
76+
// Verify the native artifact exists before any test in the suite
77+
// attempts to load DBSQLClient (which transitively imports
78+
// SeaNativeLoader's module-level require of the .node). Skip with
79+
// a clear message so a developer sees the actionable instruction.
80+
const nodeArtifact = path.resolve(
81+
process.cwd(),
82+
'native/sea/index.linux-x64-gnu.node',
83+
);
84+
if (!fs.existsSync(nodeArtifact)) {
85+
// eslint-disable-next-line no-console
86+
console.warn(
87+
`[sea fetch-all e2e] skipping: native binary not built. ` +
88+
`Run \`yarn build:native\` first.`,
89+
);
90+
// eslint-disable-next-line no-invalid-this
91+
this.skip();
5492
}
5593
});
5694

5795
async function connect() {
96+
// Lazy-load the facade so the suite skip-gate runs first. See the
97+
// top-of-file comment for why this matters.
98+
const { DBSQLClient } = requireFromHere('../../../lib') as typeof import('../../../lib');
5899
const client = new DBSQLClient();
59100
// `useSEA` is an internal opt-in flag (not on the public TS
60101
// surface; see `lib/contracts/InternalConnectionOptions.ts`).
@@ -151,4 +192,70 @@ describe('SEA fetchAll — Array<object> row drain', function suite() {
151192
await client.close();
152193
}
153194
});
195+
196+
// ─── Edge cases (DA round-1 M1 — drain-twice / drain-after-close) ───
197+
198+
it('drain-twice — fetchAll on an already-drained operation returns []', async () => {
199+
// After a successful fetchAll drains the cursor to end-of-stream,
200+
// a second fetchAll on the same operation must produce an empty
201+
// array (matching Thrift's `DBSQLOperation.fetchAll` semantics:
202+
// hasMoreRows is false, so the do/while body executes zero
203+
// iterations and the empty array.flat() yields []).
204+
const client = await connect();
205+
try {
206+
const session = await client.openSession();
207+
try {
208+
const operation = await session.executeStatement('SELECT * FROM range(0, 10)');
209+
try {
210+
const first = await operation.fetchAll();
211+
expect(first.length).to.equal(10);
212+
// Second drain — must not throw, must return [].
213+
const second = await operation.fetchAll();
214+
expect(second).to.be.an('array');
215+
expect(second.length).to.equal(0);
216+
} finally {
217+
await operation.close();
218+
}
219+
} finally {
220+
await session.close();
221+
}
222+
} finally {
223+
await client.close();
224+
}
225+
});
226+
227+
it('drain-after-close — fetchAll on a closed operation throws OperationStateError', async () => {
228+
// Closing the operation invalidates the underlying napi handle.
229+
// The facade should surface a typed error rather than crash or
230+
// return garbage. Mirrors Thrift's behaviour: closed operations
231+
// reject subsequent reads with an OperationStateError that the
232+
// application can catch and surface.
233+
const client = await connect();
234+
try {
235+
const session = await client.openSession();
236+
try {
237+
const operation = await session.executeStatement('SELECT * FROM range(0, 10)');
238+
await operation.close();
239+
let threw = false;
240+
try {
241+
await operation.fetchAll();
242+
} catch (err) {
243+
threw = true;
244+
// We don't pin the exact error class here because the SEA
245+
// backend's closed-state error path collapses to either an
246+
// `OperationStateError` or a kernel-envelope-decoded error
247+
// depending on whether the close had already reached the
248+
// facade-level lifecycle flag or only the napi layer. Both
249+
// are acceptable; what's not acceptable is a silent return
250+
// or an unhandled exception type.
251+
expect(err).to.be.an.instanceof(Error);
252+
}
253+
expect(threw, 'fetchAll on closed operation must throw').to.equal(true);
254+
} finally {
255+
await session.close();
256+
}
257+
} finally {
258+
await client.close();
259+
}
260+
});
154261
});

0 commit comments

Comments
 (0)