From 6ccf139ce48eac140864175077bf72aad6307e1e Mon Sep 17 00:00:00 2001 From: Chris Sidi Date: Wed, 25 Nov 2020 21:06:28 -0500 Subject: [PATCH 1/4] Remove spyon, use readable stream to test pipeResponseToFile --- packages/artifact/__tests__/download.test.ts | 150 +++++++++++++------ 1 file changed, 108 insertions(+), 42 deletions(-) diff --git a/packages/artifact/__tests__/download.test.ts b/packages/artifact/__tests__/download.test.ts index 40ab58cb75..4ad7d13e66 100644 --- a/packages/artifact/__tests__/download.test.ts +++ b/packages/artifact/__tests__/download.test.ts @@ -12,6 +12,9 @@ import { ListArtifactsResponse, QueryArtifactResponse } from '../src/internal/contracts' +import * as stream from 'stream' +import {gzip} from 'zlib' +import {promisify} from 'util' const root = path.join(__dirname, '_temp', 'artifact-download-tests') @@ -114,33 +117,45 @@ describe('Download Tests', () => { }) it('Test downloading an individual artifact with gzip', async () => { - setupDownloadItemResponse(true, 200) + const response = 'gzip worked on the first try\n' + const targetPath = path.join(root, 'FileA.txt') + + setupDownloadItemResponse(true, 200, response) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] items.push({ sourceLocation: `${configVariables.getRuntimeUrl()}_apis/resources/Containers/13?itemPath=my-artifact%2FFileA.txt`, - targetPath: path.join(root, 'FileA.txt') + targetPath }) await expect( downloadHttpClient.downloadSingleArtifact(items) ).resolves.not.toThrow() + + const size = (await fs.stat(targetPath)).size + expect(size).toEqual(response.length) }) it('Test downloading an individual artifact without gzip', async () => { - setupDownloadItemResponse(false, 200) + const response = 'plaintext worked on the first try\n' + const targetPath = path.join(root, 'FileB.txt') + + setupDownloadItemResponse(false, 200, response) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] items.push({ sourceLocation: `${configVariables.getRuntimeUrl()}_apis/resources/Containers/13?itemPath=my-artifact%2FFileB.txt`, - targetPath: path.join(root, 'FileB.txt') + targetPath }) await expect( downloadHttpClient.downloadSingleArtifact(items) ).resolves.not.toThrow() + + const size = (await fs.stat(targetPath)).size + expect(size).toEqual(response.length) }) it('Test retryable status codes during artifact download', async () => { @@ -148,18 +163,24 @@ describe('Download Tests', () => { // the download should successfully finish const retryableStatusCodes = [429, 502, 503, 504] for (const statusCode of retryableStatusCodes) { - setupDownloadItemResponse(false, statusCode) + const response = 'try, try again\n' + const targetPath = path.join(root, `FileC-${statusCode}.txt`) + + setupDownloadItemResponse(false, statusCode, response) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] items.push({ sourceLocation: `${configVariables.getRuntimeUrl()}_apis/resources/Containers/13?itemPath=my-artifact%2FFileC.txt`, - targetPath: path.join(root, 'FileC.txt') + targetPath }) await expect( downloadHttpClient.downloadSingleArtifact(items) ).resolves.not.toThrow() + + const size = (await fs.stat(targetPath)).size + expect(size).toEqual(response.length) } }) @@ -227,51 +248,96 @@ describe('Download Tests', () => { */ function setupDownloadItemResponse( isGzip: boolean, - firstHttpResponseCode: number + firstHttpResponseCode: number, + response: string | Buffer ): void { - jest - .spyOn(DownloadHttpClient.prototype, 'pipeResponseToFile') - .mockImplementationOnce(async () => { - return new Promise(resolve => { - resolve() - }) - }) - - jest + const spyInstance = jest .spyOn(HttpClient.prototype, 'get') .mockImplementationOnce(async () => { - const mockMessage = new http.IncomingMessage(new net.Socket()) - mockMessage.statusCode = firstHttpResponseCode - if (isGzip) { - mockMessage.headers = { - 'content-type': 'gzip' + if (firstHttpResponseCode === 200) { + return { + message: getDownloadResponseMessage( + firstHttpResponseCode, + isGzip, + await constructResponse(isGzip, response) + ), + readBody: emptyMockReadBody } - } - - return new Promise(resolve => { - resolve({ - message: mockMessage, + } else { + return { + message: getDownloadResponseMessage( + firstHttpResponseCode, + false, + null + ), readBody: emptyMockReadBody - }) - }) - }) - .mockImplementationOnce(async () => { - // chained response, if the HTTP GET function gets called again, return a successful response - const mockMessage = new http.IncomingMessage(new net.Socket()) - mockMessage.statusCode = 200 - if (isGzip) { - mockMessage.headers = { - 'content-type': 'gzip' } } + }) - return new Promise(resolve => { - resolve({ - message: mockMessage, - readBody: emptyMockReadBody - }) - }) + // set up a second mock only if we expect a retry. Otherwise this mock will affect other tests. + if (firstHttpResponseCode !== 200) { + spyInstance.mockImplementationOnce(async () => { + // chained response, if the HTTP GET function gets called again, return a successful response + return { + message: getDownloadResponseMessage( + 200, + isGzip, + await constructResponse(isGzip, response) + ), + readBody: emptyMockReadBody + } }) + } + } + + async function constructResponse( + isGzip: boolean, + plaintext: string | Buffer + ): Promise { + if (isGzip) { + return await promisify(gzip)(plaintext) + } else if (typeof plaintext === 'string') { + return Buffer.from(plaintext) + } else { + return plaintext + } + } + + function getDownloadResponseMessage( + httpResponseCode: number, + isGzip: boolean, + response: Buffer | null + ): http.IncomingMessage { + let readCallCount = 0 + const mockMessage = new stream.Readable({ + read(size) { + switch (readCallCount++) { + case 0: + if (!!response && response.byteLength > size) { + throw new Error( + `test response larger than requested size (${size})` + ) + } + this.push(response) + break + + default: + // end the stream + this.push(null) + break + } + } + }) + + mockMessage.statusCode = httpResponseCode + mockMessage.headers = {} + + if (isGzip) { + mockMessage.headers['content-encoding'] = 'gzip' + } + + return mockMessage } /** From d617c27e1f9038922b594c01ee5db1ead5fa6cd0 Mon Sep 17 00:00:00 2001 From: Chris Sidi Date: Mon, 30 Nov 2020 10:59:48 -0500 Subject: [PATCH 2/4] Check file contents --- packages/artifact/__tests__/download.test.ts | 48 +++++++++++++------- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/packages/artifact/__tests__/download.test.ts b/packages/artifact/__tests__/download.test.ts index 4ad7d13e66..305bfa8a50 100644 --- a/packages/artifact/__tests__/download.test.ts +++ b/packages/artifact/__tests__/download.test.ts @@ -17,6 +17,7 @@ import {gzip} from 'zlib' import {promisify} from 'util' const root = path.join(__dirname, '_temp', 'artifact-download-tests') +const defaultEncoding = 'utf8' jest.mock('../src/internal/config-variables') jest.mock('@actions/http-client') @@ -117,10 +118,13 @@ describe('Download Tests', () => { }) it('Test downloading an individual artifact with gzip', async () => { - const response = 'gzip worked on the first try\n' + const fileContents = Buffer.from( + 'gzip worked on the first try\n', + defaultEncoding + ) const targetPath = path.join(root, 'FileA.txt') - setupDownloadItemResponse(true, 200, response) + setupDownloadItemResponse(true, 200, fileContents) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -133,15 +137,17 @@ describe('Download Tests', () => { downloadHttpClient.downloadSingleArtifact(items) ).resolves.not.toThrow() - const size = (await fs.stat(targetPath)).size - expect(size).toEqual(response.length) + await checkDestinationFile(targetPath, fileContents) }) it('Test downloading an individual artifact without gzip', async () => { - const response = 'plaintext worked on the first try\n' + const fileContents = Buffer.from( + 'plaintext worked on the first try\n', + defaultEncoding + ) const targetPath = path.join(root, 'FileB.txt') - setupDownloadItemResponse(false, 200, response) + setupDownloadItemResponse(false, 200, fileContents) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -154,8 +160,7 @@ describe('Download Tests', () => { downloadHttpClient.downloadSingleArtifact(items) ).resolves.not.toThrow() - const size = (await fs.stat(targetPath)).size - expect(size).toEqual(response.length) + await checkDestinationFile(targetPath, fileContents) }) it('Test retryable status codes during artifact download', async () => { @@ -163,10 +168,10 @@ describe('Download Tests', () => { // the download should successfully finish const retryableStatusCodes = [429, 502, 503, 504] for (const statusCode of retryableStatusCodes) { - const response = 'try, try again\n' + const fileContents = Buffer.from('try, try again\n', defaultEncoding) const targetPath = path.join(root, `FileC-${statusCode}.txt`) - setupDownloadItemResponse(false, statusCode, response) + setupDownloadItemResponse(false, statusCode, fileContents) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -179,8 +184,7 @@ describe('Download Tests', () => { downloadHttpClient.downloadSingleArtifact(items) ).resolves.not.toThrow() - const size = (await fs.stat(targetPath)).size - expect(size).toEqual(response.length) + await checkDestinationFile(targetPath, fileContents) } }) @@ -249,7 +253,7 @@ describe('Download Tests', () => { function setupDownloadItemResponse( isGzip: boolean, firstHttpResponseCode: number, - response: string | Buffer + fileContents: Buffer ): void { const spyInstance = jest .spyOn(HttpClient.prototype, 'get') @@ -259,7 +263,7 @@ describe('Download Tests', () => { message: getDownloadResponseMessage( firstHttpResponseCode, isGzip, - await constructResponse(isGzip, response) + await constructResponse(isGzip, fileContents) ), readBody: emptyMockReadBody } @@ -283,7 +287,7 @@ describe('Download Tests', () => { message: getDownloadResponseMessage( 200, isGzip, - await constructResponse(isGzip, response) + await constructResponse(isGzip, fileContents) ), readBody: emptyMockReadBody } @@ -293,12 +297,12 @@ describe('Download Tests', () => { async function constructResponse( isGzip: boolean, - plaintext: string | Buffer + plaintext: Buffer | string ): Promise { if (isGzip) { return await promisify(gzip)(plaintext) } else if (typeof plaintext === 'string') { - return Buffer.from(plaintext) + return Buffer.from(plaintext, defaultEncoding) } else { return plaintext } @@ -413,4 +417,14 @@ describe('Download Tests', () => { }) }) } + + async function checkDestinationFile( + targetPath: string, + expectedContents: Buffer + ): Promise { + const fileContents = await fs.readFile(targetPath) + + expect(fileContents.byteLength).toEqual(expectedContents.byteLength) + expect(fileContents.equals(expectedContents)).toBeTruthy() + } }) From aa9ed1d4250f9a6c88cc31c4fefb900aa243a325 Mon Sep 17 00:00:00 2001 From: Chris Sidi Date: Wed, 25 Nov 2020 23:04:07 -0500 Subject: [PATCH 3/4] Retry artifact download when response stream is truncated --- packages/artifact/__tests__/download.test.ts | 71 +++++++++++++++++-- .../src/internal/download-http-client.ts | 6 ++ 2 files changed, 71 insertions(+), 6 deletions(-) diff --git a/packages/artifact/__tests__/download.test.ts b/packages/artifact/__tests__/download.test.ts index 305bfa8a50..81759d4666 100644 --- a/packages/artifact/__tests__/download.test.ts +++ b/packages/artifact/__tests__/download.test.ts @@ -124,7 +124,7 @@ describe('Download Tests', () => { ) const targetPath = path.join(root, 'FileA.txt') - setupDownloadItemResponse(true, 200, fileContents) + setupDownloadItemResponse(true, 200, false, fileContents) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -147,7 +147,7 @@ describe('Download Tests', () => { ) const targetPath = path.join(root, 'FileB.txt') - setupDownloadItemResponse(false, 200, fileContents) + setupDownloadItemResponse(false, 200, false, fileContents) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -171,7 +171,7 @@ describe('Download Tests', () => { const fileContents = Buffer.from('try, try again\n', defaultEncoding) const targetPath = path.join(root, `FileC-${statusCode}.txt`) - setupDownloadItemResponse(false, statusCode, fileContents) + setupDownloadItemResponse(false, statusCode, false, fileContents) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -188,6 +188,52 @@ describe('Download Tests', () => { } }) + it('Test retry on truncated response with gzip', async () => { + const fileContents = Buffer.from( + 'Sometimes gunzip fails on the first try\n', + defaultEncoding + ) + const targetPath = path.join(root, 'FileD.txt') + + setupDownloadItemResponse(true, 200, true, fileContents) + const downloadHttpClient = new DownloadHttpClient() + + const items: DownloadItem[] = [] + items.push({ + sourceLocation: `${configVariables.getRuntimeUrl()}_apis/resources/Containers/13?itemPath=my-artifact%2FFileD.txt`, + targetPath + }) + + await expect( + downloadHttpClient.downloadSingleArtifact(items) + ).resolves.not.toThrow() + + await checkDestinationFile(targetPath, fileContents) + }) + + it('Test retry on truncated response without gzip', async () => { + const fileContents = Buffer.from( + 'You have to inspect the content-length header to know if you got everything\n', + defaultEncoding + ) + const targetPath = path.join(root, 'FileE.txt') + + setupDownloadItemResponse(false, 200, true, fileContents) + const downloadHttpClient = new DownloadHttpClient() + + const items: DownloadItem[] = [] + items.push({ + sourceLocation: `${configVariables.getRuntimeUrl()}_apis/resources/Containers/13?itemPath=my-artifact%2FFileD.txt`, + targetPath + }) + + await expect( + downloadHttpClient.downloadSingleArtifact(items) + ).resolves.not.toThrow() + + await checkDestinationFile(targetPath, fileContents) + }) + /** * Helper used to setup mocking for the HttpClient */ @@ -253,17 +299,24 @@ describe('Download Tests', () => { function setupDownloadItemResponse( isGzip: boolean, firstHttpResponseCode: number, + truncateFirstResponse: boolean, fileContents: Buffer ): void { const spyInstance = jest .spyOn(HttpClient.prototype, 'get') .mockImplementationOnce(async () => { if (firstHttpResponseCode === 200) { + const fullResponse = await constructResponse(isGzip, fileContents) + const actualResponse = truncateFirstResponse + ? fullResponse.subarray(0, 3) + : fullResponse + return { message: getDownloadResponseMessage( firstHttpResponseCode, isGzip, - await constructResponse(isGzip, fileContents) + fullResponse.length, + actualResponse ), readBody: emptyMockReadBody } @@ -272,6 +325,7 @@ describe('Download Tests', () => { message: getDownloadResponseMessage( firstHttpResponseCode, false, + 0, null ), readBody: emptyMockReadBody @@ -283,11 +337,13 @@ describe('Download Tests', () => { if (firstHttpResponseCode !== 200) { spyInstance.mockImplementationOnce(async () => { // chained response, if the HTTP GET function gets called again, return a successful response + const fullResponse = await constructResponse(isGzip, fileContents) return { message: getDownloadResponseMessage( 200, isGzip, - await constructResponse(isGzip, fileContents) + fullResponse.length, + fullResponse ), readBody: emptyMockReadBody } @@ -311,6 +367,7 @@ describe('Download Tests', () => { function getDownloadResponseMessage( httpResponseCode: number, isGzip: boolean, + contentLength: number, response: Buffer | null ): http.IncomingMessage { let readCallCount = 0 @@ -335,7 +392,9 @@ describe('Download Tests', () => { }) mockMessage.statusCode = httpResponseCode - mockMessage.headers = {} + mockMessage.headers = { + 'content-length': contentLength.toString() + } if (isGzip) { mockMessage.headers['content-encoding'] = 'gzip' diff --git a/packages/artifact/src/internal/download-http-client.ts b/packages/artifact/src/internal/download-http-client.ts index 70ea4e447a..3b00872eda 100644 --- a/packages/artifact/src/internal/download-http-client.ts +++ b/packages/artifact/src/internal/download-http-client.ts @@ -264,6 +264,12 @@ export class DownloadHttpClient { const gunzip = zlib.createGunzip() response.message .pipe(gunzip) + .on('error', error => { + core.error( + `An error has been encountered while attempting to decompress a file` + ) + reject(error) + }) .pipe(destinationStream) .on('close', () => { resolve() From 7ade2bba7d34a9c91857897fd8221186ecd9301a Mon Sep 17 00:00:00 2001 From: Chris Sidi Date: Thu, 26 Nov 2020 01:16:06 -0500 Subject: [PATCH 4/4] More error handling --- .../src/internal/download-http-client.ts | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/packages/artifact/src/internal/download-http-client.ts b/packages/artifact/src/internal/download-http-client.ts index 3b00872eda..64631c5c1f 100644 --- a/packages/artifact/src/internal/download-http-client.ts +++ b/packages/artifact/src/internal/download-http-client.ts @@ -263,12 +263,20 @@ export class DownloadHttpClient { if (isGzip) { const gunzip = zlib.createGunzip() response.message + .on('error', error => { + core.error( + `An error occurred while attempting to read the response stream` + ) + reject(error) + gunzip.close() + }) .pipe(gunzip) .on('error', error => { core.error( - `An error has been encountered while attempting to decompress a file` + `An error occurred while attempting to decompress the response stream` ) reject(error) + destinationStream.close() }) .pipe(destinationStream) .on('close', () => { @@ -276,19 +284,26 @@ export class DownloadHttpClient { }) .on('error', error => { core.error( - `An error has been encountered while decompressing and writing a downloaded file to ${destinationStream.path}` + `An error occurred while writing a downloaded file to ${destinationStream.path}` ) reject(error) }) } else { response.message + .on('error', error => { + core.error( + `An error occurred while attempting to read the response stream` + ) + reject(error) + destinationStream.close() + }) .pipe(destinationStream) .on('close', () => { resolve() }) .on('error', error => { core.error( - `An error has been encountered while writing a downloaded file to ${destinationStream.path}` + `An error occurred while writing a downloaded file to ${destinationStream.path}` ) reject(error) })