Skip to content

Commit df3e568

Browse files
committed
test(fetch): stub internal drain primitives in fetchAll unit test
The fetchAll() drain holds the per-operation fetch lock across the whole loop and therefore calls the non-locking `fetchChunkInternal`/`hasMoreRowsInternal` primitives (calling the public `fetchChunk`/`hasMoreRows`, which re-acquire the same lock, would self-deadlock). The existing unit test stubbed the *public* methods, which the refactored drain no longer calls — so the stubs were bypassed, the real internals ran with no data source, and the test timed out (2000ms). Stub the internal primitives the drain actually invokes. Behavior asserted is unchanged (fetchAll drains all chunks and returns all rows). Test is explicitly implementation-specific (see its comment). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
1 parent bec8a52 commit df3e568

1 file changed

Lines changed: 15 additions & 9 deletions

File tree

tests/unit/DBSQLOperation.test.ts

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1008,20 +1008,26 @@ describe('DBSQLOperation', () => {
10081008
const originalData = [1, 2, 3, 4, 5, 6, 7, 8, 9, 0];
10091009

10101010
const tempData = [...originalData];
1011-
const fetchChunkStub = sinon.stub(operation, 'fetchChunk').callsFake(async (): Promise<Array<any>> => {
1012-
return tempData.splice(0, 3);
1013-
});
1014-
const hasMoreRowsStub = sinon.stub(operation, 'hasMoreRows').callsFake(async () => {
1011+
// Warning: this check is implementation-specific.
1012+
// `fetchAll` holds the per-operation fetch lock across the entire drain, so
1013+
// it calls the *non-locking* internal primitives (`fetchChunkInternal` /
1014+
// `hasMoreRowsInternal`) rather than the public `fetchChunk` / `hasMoreRows`
1015+
// (which re-acquire the same lock and would self-deadlock). Stub the
1016+
// internals that the drain loop actually invokes.
1017+
const fetchChunkStub = sinon
1018+
.stub(operation as any, 'fetchChunkInternal')
1019+
.callsFake(async (): Promise<Array<any>> => {
1020+
return tempData.splice(0, 3);
1021+
});
1022+
const hasMoreRowsStub = sinon.stub(operation as any, 'hasMoreRowsInternal').callsFake(async () => {
10151023
return tempData.length > 0;
10161024
});
10171025

10181026
const fetchedData = await operation.fetchAll();
10191027

1020-
// Warning: this check is implementation-specific
1021-
// `fetchAll` should wait for operation to complete. In current implementation
1022-
// it does so by calling `fetchChunk` at least once, which internally does
1023-
// all the job. But since here we stub `fetchChunk` it won't really wait,
1024-
// therefore here we ensure it was called at least once
1028+
// `fetchAll` should wait for the operation to complete; in the current
1029+
// implementation it does so by draining via `fetchChunkInternal` at least
1030+
// once, which internally does all the work.
10251031
expect(fetchChunkStub.callCount).to.be.gte(1);
10261032

10271033
expect(fetchChunkStub.called).to.be.true;

0 commit comments

Comments
 (0)