Skip to content

Commit f9ca5b7

Browse files
iclantonclaude
andcommitted
Fix unit test issues in streaming cache tests
- Fix credential fallback test: mock CredentialCache so cached credentials are available, making the test actually validate the stream-body guard (previously, the test passed trivially because _tryGetCredentialsAsync would throw before making a second request) - Fix 504 statusText from 'BadGateway' to 'Gateway Timeout' - Replace fragile S3 upload snapshot that captured Readable internals (breaks on Node.js upgrades) with targeted assertions on URL, verb, headers, and body identity - Replace fail() + try/catch with expect().rejects.toThrow() - Move Readable import to top of S3 test file instead of per-test dynamic import Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 4577a64 commit f9ca5b7

File tree

3 files changed

+65
-104
lines changed

3 files changed

+65
-104
lines changed

rush-plugins/rush-amazon-s3-build-cache-plugin/src/test/AmazonS3Client.test.ts

Lines changed: 46 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ jest.mock('@rushstack/rush-sdk/lib/utilities/WebClient', () => {
55
return jest.requireActual('@microsoft/rush-lib/lib/utilities/WebClient');
66
});
77

8+
import { Readable } from 'node:stream';
9+
810
import { ConsoleTerminalProvider, Terminal } from '@rushstack/terminal';
911
import { WebClient } from '@rushstack/rush-sdk/lib/utilities/WebClient';
1012

@@ -664,21 +666,18 @@ describe(AmazonS3Client.name, () => {
664666
status: number,
665667
statusText?: string
666668
): Promise<{ result: NodeJS.ReadableStream | undefined; spy: jest.SpyInstance }> {
667-
const { Readable } = await import('node:stream');
668669
const mockStream = new Readable({ read() {} });
669670

670-
const spy: jest.SpyInstance = jest
671-
.spyOn(WebClient.prototype, 'fetchStreamAsync')
672-
.mockReturnValue(
673-
Promise.resolve({
674-
stream: mockStream,
675-
headers: {},
676-
status,
677-
statusText,
678-
ok: status >= 200 && status < 300,
679-
redirected: false
680-
})
681-
);
671+
const spy: jest.SpyInstance = jest.spyOn(WebClient.prototype, 'fetchStreamAsync').mockReturnValue(
672+
Promise.resolve({
673+
stream: mockStream,
674+
headers: {},
675+
status,
676+
statusText,
677+
ok: status >= 200 && status < 300,
678+
redirected: false
679+
})
680+
);
682681

683682
const s3Client: AmazonS3Client = new AmazonS3Client(credentials, options, webClient, terminal);
684683
const result = await s3Client.getObjectStreamAsync(objectName);
@@ -722,7 +721,6 @@ describe(AmazonS3Client.name, () => {
722721

723722
describe('Uploading an object stream', () => {
724723
it('Throws an error if credentials are not provided', async () => {
725-
const { Readable } = await import('node:stream');
726724
const s3Client: AmazonS3Client = new AmazonS3Client(
727725
undefined,
728726
{ s3Endpoint: 'http://foo.bar.baz', ...DUMMY_OPTIONS_WITHOUT_ENDPOINT },
@@ -731,31 +729,25 @@ describe(AmazonS3Client.name, () => {
731729
);
732730

733731
const mockStream = new Readable({ read() {} });
734-
try {
735-
await s3Client.uploadObjectStreamAsync('temp', mockStream);
736-
fail('Expected an exception to be thrown');
737-
} catch (e) {
738-
expect(e).toMatchSnapshot();
739-
}
732+
await expect(s3Client.uploadObjectStreamAsync('temp', mockStream)).rejects.toThrow(
733+
'Credentials are required to upload objects to S3.'
734+
);
740735
});
741736

742737
it('Uploads a stream successfully', async () => {
743-
const { Readable } = await import('node:stream');
744738
const mockStream = new Readable({ read() {} });
745739
const responseStream = new Readable({ read() {} });
746740

747-
const spy: jest.SpyInstance = jest
748-
.spyOn(WebClient.prototype, 'fetchStreamAsync')
749-
.mockReturnValue(
750-
Promise.resolve({
751-
stream: responseStream,
752-
headers: {},
753-
status: 200,
754-
statusText: 'OK',
755-
ok: true,
756-
redirected: false
757-
})
758-
);
741+
const spy: jest.SpyInstance = jest.spyOn(WebClient.prototype, 'fetchStreamAsync').mockReturnValue(
742+
Promise.resolve({
743+
stream: responseStream,
744+
headers: {},
745+
status: 200,
746+
statusText: 'OK',
747+
ok: true,
748+
redirected: false
749+
})
750+
);
759751

760752
const s3Client: AmazonS3Client = new AmazonS3Client(
761753
{
@@ -771,27 +763,33 @@ describe(AmazonS3Client.name, () => {
771763
await s3Client.uploadObjectStreamAsync('abc123', mockStream);
772764

773765
expect(spy).toHaveBeenCalledTimes(1);
774-
expect(spy.mock.calls[0]).toMatchSnapshot();
766+
// Assert on URL and request options without snapshotting Readable internals,
767+
// which are fragile across Node.js versions
768+
const [url, options] = spy.mock.calls[0];
769+
expect(url).toBe('http://localhost:9000/abc123');
770+
expect(options.verb).toBe('PUT');
771+
expect(options.body).toBe(mockStream);
772+
expect(options.headers['x-amz-content-sha256']).toBe('UNSIGNED-PAYLOAD');
773+
expect(options.headers['x-amz-date']).toBe('20200418T123242Z');
774+
// eslint-disable-next-line dot-notation
775+
expect(options.headers['Authorization']).toContain('AWS4-HMAC-SHA256');
775776
spy.mockRestore();
776777
});
777778

778779
it('Does not retry on failure (stream consumed)', async () => {
779-
const { Readable } = await import('node:stream');
780780
const mockStream = new Readable({ read() {} });
781781
const responseStream = new Readable({ read() {} });
782782

783-
const spy: jest.SpyInstance = jest
784-
.spyOn(WebClient.prototype, 'fetchStreamAsync')
785-
.mockReturnValue(
786-
Promise.resolve({
787-
stream: responseStream,
788-
headers: {},
789-
status: 500,
790-
statusText: 'InternalServerError',
791-
ok: false,
792-
redirected: false
793-
})
794-
);
783+
const spy: jest.SpyInstance = jest.spyOn(WebClient.prototype, 'fetchStreamAsync').mockReturnValue(
784+
Promise.resolve({
785+
stream: responseStream,
786+
headers: {},
787+
status: 500,
788+
statusText: 'InternalServerError',
789+
ok: false,
790+
redirected: false
791+
})
792+
);
795793

796794
const s3Client: AmazonS3Client = new AmazonS3Client(
797795
{
@@ -804,12 +802,7 @@ describe(AmazonS3Client.name, () => {
804802
terminal
805803
);
806804

807-
try {
808-
await s3Client.uploadObjectStreamAsync('abc123', mockStream);
809-
fail('Expected an exception to be thrown');
810-
} catch (e) {
811-
expect((e as Error).message).toContain('500');
812-
}
805+
await expect(s3Client.uploadObjectStreamAsync('abc123', mockStream)).rejects.toThrow('500');
813806

814807
// Only 1 call - no retry for streams
815808
expect(spy).toHaveBeenCalledTimes(1);

rush-plugins/rush-amazon-s3-build-cache-plugin/src/test/__snapshots__/AmazonS3Client.test.ts.snap

Lines changed: 0 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -563,40 +563,3 @@ Array [
563563
]
564564
`;
565565

566-
exports[`AmazonS3Client Streaming requests Uploading an object stream Throws an error if credentials are not provided 1`] = `[Error: Credentials are required to upload objects to S3.]`;
567-
568-
exports[`AmazonS3Client Streaming requests Uploading an object stream Uploads a stream successfully 1`] = `
569-
Array [
570-
"http://localhost:9000/abc123",
571-
Object {
572-
"body": Readable {
573-
"_events": Object {
574-
"close": undefined,
575-
"data": undefined,
576-
"end": undefined,
577-
"error": undefined,
578-
"readable": undefined,
579-
},
580-
"_maxListeners": undefined,
581-
"_read": [Function],
582-
"_readableState": ReadableState {
583-
"awaitDrainWriters": null,
584-
"buffer": Array [],
585-
"bufferIndex": 0,
586-
"highWaterMark": 65536,
587-
"length": 0,
588-
"pipes": Array [],
589-
Symbol(kState): 1052940,
590-
},
591-
Symbol(shapeMode): true,
592-
Symbol(kCapture): false,
593-
},
594-
"headers": Object {
595-
"Authorization": "AWS4-HMAC-SHA256 Credential=accessKeyId/20200418/us-east-1/s3/aws4_request,SignedHeaders=host;x-amz-content-sha256;x-amz-date,Signature=bfc5ee97ccfdb44b7f351c0b550a1ac9c2cdb661606dbba8a74c11be6b5b2b72",
596-
"x-amz-content-sha256": "UNSIGNED-PAYLOAD",
597-
"x-amz-date": "20200418T123242Z",
598-
},
599-
"verb": "PUT",
600-
},
601-
]
602-
`;

rush-plugins/rush-http-build-cache-plugin/src/test/HttpBuildCacheProvider.test.ts

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ jest.mock('@rushstack/rush-sdk/lib/utilities/WebClient', () => {
88
import { Readable } from 'node:stream';
99

1010
import { type RushSession, EnvironmentConfiguration } from '@rushstack/rush-sdk';
11+
import { type ICredentialCacheEntry, CredentialCache } from '@rushstack/credential-cache';
1112
import { StringBufferTerminalProvider, Terminal } from '@rushstack/terminal';
1213
import { WebClient } from '@rushstack/rush-sdk/lib/utilities/WebClient';
1314

@@ -113,7 +114,7 @@ Array [
113114
});
114115
mocked(fetchFn).mockResolvedValueOnce({
115116
status: 504,
116-
statusText: 'BadGateway',
117+
statusText: 'Gateway Timeout',
117118
ok: false
118119
});
119120

@@ -149,7 +150,7 @@ Array [
149150
"[ debug] [http-build-cache] request: GET https://buildcache.example.acme.com/some-key unknown bytes[n]",
150151
"[ debug] [http-build-cache] request: GET https://buildcache.example.acme.com/some-key unknown bytes[n]",
151152
"[ debug] [http-build-cache] request: GET https://buildcache.example.acme.com/some-key unknown bytes[n]",
152-
"[warning] Could not get cache entry: HTTP 504: BadGateway[n]",
153+
"[warning] Could not get cache entry: HTTP 504: Gateway Timeout[n]",
153154
]
154155
`);
155156
});
@@ -183,11 +184,7 @@ Array [
183184
const session: RushSession = {} as RushSession;
184185
const provider = new HttpBuildCacheProvider(EXAMPLE_OPTIONS, session); // write not allowed
185186

186-
const result = await provider.trySetCacheEntryBufferAsync(
187-
terminal,
188-
'some-key',
189-
Buffer.from('data')
190-
);
187+
const result = await provider.trySetCacheEntryBufferAsync(terminal, 'some-key', Buffer.from('data'));
191188

192189
expect(result).toBe(false);
193190
expect(fetchFn).not.toHaveBeenCalled();
@@ -235,11 +232,7 @@ Array [
235232
ok: false
236233
});
237234

238-
const result = await provider.trySetCacheEntryBufferAsync(
239-
terminal,
240-
'some-key',
241-
Buffer.from('data')
242-
);
235+
const result = await provider.trySetCacheEntryBufferAsync(terminal, 'some-key', Buffer.from('data'));
243236

244237
expect(result).toBe(false);
245238
expect(fetchFn).toHaveBeenCalledTimes(3);
@@ -316,7 +309,7 @@ Array [
316309
});
317310
mocked(streamFetchFn).mockResolvedValueOnce({
318311
status: 504,
319-
statusText: 'BadGateway',
312+
statusText: 'Gateway Timeout',
320313
ok: false,
321314
stream: createMockStream()
322315
});
@@ -393,7 +386,19 @@ Array [
393386
});
394387

395388
it('skips credential fallback for stream bodies on 4xx', async () => {
389+
// No credential in env for the first attempt
396390
jest.spyOn(EnvironmentConfiguration, 'buildCacheCredential', 'get').mockReturnValue(undefined);
391+
// But credentials ARE available in the credential cache — without the stream-body
392+
// guard, the credential fallback would resolve these and make a second HTTP request
393+
// with the already-consumed stream body.
394+
jest
395+
.spyOn(CredentialCache, 'usingAsync')
396+
// eslint-disable-next-line @typescript-eslint/naming-convention
397+
.mockImplementation(async (_options, fn) => {
398+
await (fn as (cache: CredentialCache) => Promise<void>)({
399+
tryGetCacheEntry: (): ICredentialCacheEntry => ({ credential: 'cached-token' })
400+
} as unknown as CredentialCache);
401+
});
397402

398403
const session: RushSession = {} as RushSession;
399404
const provider = new HttpBuildCacheProvider(WRITE_ALLOWED_OPTIONS, session);
@@ -412,7 +417,7 @@ Array [
412417
const result = await provider.trySetCacheEntryStreamAsync(terminal, 'some-key', entryStream);
413418

414419
expect(result).toBe(false);
415-
// Should only be called once (no credential fallback retry)
420+
// Should only be called once (no credential fallback retry with consumed stream)
416421
expect(streamFetchFn).toHaveBeenCalledTimes(1);
417422
});
418423
});

0 commit comments

Comments
 (0)