From d2b2399bd2fce877e9dd828711f74b83306a602b Mon Sep 17 00:00:00 2001 From: Aiqiao Yan Date: Fri, 15 May 2020 12:18:50 -0400 Subject: [PATCH] React to feedback --- .github/workflows/artifact-tests.yml | 8 ++-- .github/workflows/cache-tests.yml | 8 ++-- .../artifact/__tests__}/test-artifact-file.sh | 0 packages/cache/README.md | 12 ++--- packages/cache/RELEASES.md | 2 +- packages/cache/__tests__/cacheUtils.test.ts | 19 ++------ .../cache/__tests__}/create-cache-files.sh | 0 packages/cache/__tests__/restoreCache.test.ts | 44 +++++++------------ packages/cache/__tests__/saveCache.test.ts | 12 ++++- .../cache/__tests__}/verify-cache-files.sh | 0 packages/cache/package-lock.json | 2 +- packages/cache/package.json | 2 +- packages/cache/src/cache.ts | 14 +++--- .../cache/src/internal/cacheHttpClient.ts | 4 +- packages/cache/src/internal/cacheUtils.ts | 3 +- packages/cache/src/options.ts | 2 +- 16 files changed, 59 insertions(+), 73 deletions(-) rename {scripts => packages/artifact/__tests__}/test-artifact-file.sh (100%) rename {scripts => packages/cache/__tests__}/create-cache-files.sh (100%) rename {scripts => packages/cache/__tests__}/verify-cache-files.sh (100%) diff --git a/.github/workflows/artifact-tests.yml b/.github/workflows/artifact-tests.yml index 8b45404a..5fc98f7e 100644 --- a/.github/workflows/artifact-tests.yml +++ b/.github/workflows/artifact-tests.yml @@ -64,8 +64,8 @@ jobs: - name: Verify downloadArtifact() shell: bash run: | - scripts/test-artifact-file.sh "artifact-1-directory/artifact-path/world.txt" "${{ env.non-gzip-artifact-content }}" - scripts/test-artifact-file.sh "artifact-2-directory/artifact-path/gzip.txt" "${{ env.gzip-artifact-content }}" + packages/artifact/__tests__/test-artifact-file.sh "artifact-1-directory/artifact-path/world.txt" "${{ env.non-gzip-artifact-content }}" + packages/artifact/__tests__/test-artifact-file.sh "artifact-2-directory/artifact-path/gzip.txt" "${{ env.gzip-artifact-content }}" - name: Download artifacts using downloadAllArtifacts() run: | @@ -75,5 +75,5 @@ jobs: - name: Verify downloadAllArtifacts() shell: bash run: | - scripts/test-artifact-file.sh "multi-artifact-directory/my-artifact-1/artifact-path/world.txt" "${{ env.non-gzip-artifact-content }}" - scripts/test-artifact-file.sh "multi-artifact-directory/my-artifact-2/artifact-path/gzip.txt" "${{ env.gzip-artifact-content }}" \ No newline at end of file + packages/artifact/__tests__/test-artifact-file.sh "multi-artifact-directory/my-artifact-1/artifact-path/world.txt" "${{ env.non-gzip-artifact-content }}" + packages/artifact/__tests__/test-artifact-file.sh "multi-artifact-directory/my-artifact-2/artifact-path/gzip.txt" "${{ env.gzip-artifact-content }}" \ No newline at end of file diff --git a/.github/workflows/cache-tests.yml b/.github/workflows/cache-tests.yml index e63ac90c..10120ddc 100644 --- a/.github/workflows/cache-tests.yml +++ b/.github/workflows/cache-tests.yml @@ -47,11 +47,11 @@ jobs: - name: Generate files in working directory shell: bash - run: scripts/create-cache-files.sh ${{ runner.os }} test-cache + run: packages/cache/__tests__/create-cache-files.sh ${{ runner.os }} test-cache - name: Generate files outside working directory shell: bash - run: scripts/create-cache-files.sh ${{ runner.os }} ~/test-cache + run: packages/cache/__tests__/create-cache-files.sh ${{ runner.os }} ~/test-cache # We're using node -e to call the functions directly available in the @actions/cache package - name: Save cache using saveCache() @@ -65,5 +65,5 @@ jobs: - name: Verify cache shell: bash run: | - scripts/verify-cache-files.sh ${{ runner.os }} test-cache - scripts/verify-cache-files.sh ${{ runner.os }} ~/test-cache \ No newline at end of file + packages/cache/__tests__/verify-cache-files.sh ${{ runner.os }} test-cache + packages/cache/__tests__/verify-cache-files.sh ${{ runner.os }} ~/test-cache \ No newline at end of file diff --git a/scripts/test-artifact-file.sh b/packages/artifact/__tests__/test-artifact-file.sh similarity index 100% rename from scripts/test-artifact-file.sh rename to packages/artifact/__tests__/test-artifact-file.sh diff --git a/packages/cache/README.md b/packages/cache/README.md index f15d0b16..de0893d0 100644 --- a/packages/cache/README.md +++ b/packages/cache/README.md @@ -2,6 +2,10 @@ > Functions necessary for caching dependencies and build outputs to improve workflow execution time. +See ["Caching dependencies to speed up workflows"](https://help.github.com/github/automating-your-workflow-with-github-actions/caching-dependencies-to-speed-up-workflows) for how caching works. + +Note that GitHub will remove any cache entries that have not been accessed in over 7 days. There is no limit on the number of caches you can store, but the total size of all caches in a repository is limited to 5 GB. If you exceed this limit, GitHub will save your cache but will begin evicting caches until the total size is less than 5 GB. + ## Usage #### Restore Cache @@ -24,7 +28,7 @@ const cacheKey = await cache.restoreCache(paths, key, restoreKeys) #### Save Cache -Saves a cache containing the files in `paths` using the `key` provided. Function returns the cache id if the cache was save succesfully. +Saves a cache containing the files in `paths` using the `key` provided. The files would be compressed using zstandard compression algorithm if zstd is installed, otherwise gzip is used. Function returns the cache id if the cache was saved succesfully and throws an error if cache upload fails. ```js const cache = require('@actions/cache'); @@ -34,8 +38,4 @@ const paths = [ ] const key = 'npm-foobar-d5ea0750' const cacheId = await cache.saveCache(paths, key) -``` - -## Additional Documentation - -See ["Caching dependencies to speed up workflows"](https://help.github.com/github/automating-your-workflow-with-github-actions/caching-dependencies-to-speed-up-workflows). \ No newline at end of file +``` \ No newline at end of file diff --git a/packages/cache/RELEASES.md b/packages/cache/RELEASES.md index 1250c050..920a20e8 100644 --- a/packages/cache/RELEASES.md +++ b/packages/cache/RELEASES.md @@ -1,5 +1,5 @@ # @actions/cache Releases -### 1.0.0 +### 0.1.0 - Initial release \ No newline at end of file diff --git a/packages/cache/__tests__/cacheUtils.test.ts b/packages/cache/__tests__/cacheUtils.test.ts index 0a4b0f4c..25d7ca82 100644 --- a/packages/cache/__tests__/cacheUtils.test.ts +++ b/packages/cache/__tests__/cacheUtils.test.ts @@ -1,24 +1,11 @@ -import * as io from '@actions/io' import {promises as fs} from 'fs' import * as path from 'path' import * as cacheUtils from '../src/internal/cacheUtils' -jest.mock('@actions/core') -jest.mock('os') - -function getTempDir(): string { - return path.join(__dirname, '_temp', 'cacheUtils') -} - -afterAll(async () => { - delete process.env['GITHUB_WORKSPACE'] - await io.rmRF(getTempDir()) -}) - -test('getArchiveFileSize returns file size', () => { +test('getArchiveFileSizeIsBytes returns file size', () => { const filePath = path.join(__dirname, '__fixtures__', 'helloWorld.txt') - const size = cacheUtils.getArchiveFileSize(filePath) + const size = cacheUtils.getArchiveFileSizeIsBytes(filePath) expect(size).toBe(11) }) @@ -28,6 +15,8 @@ test('unlinkFile unlinks file', async () => { const testFile = path.join(testDirectory, 'test.txt') await fs.writeFile(testFile, 'hello world') + await expect(fs.stat(testFile)).resolves.not.toThrow() + await cacheUtils.unlinkFile(testFile) // This should throw as testFile should not exist diff --git a/scripts/create-cache-files.sh b/packages/cache/__tests__/create-cache-files.sh similarity index 100% rename from scripts/create-cache-files.sh rename to packages/cache/__tests__/create-cache-files.sh diff --git a/packages/cache/__tests__/restoreCache.test.ts b/packages/cache/__tests__/restoreCache.test.ts index b466b9af..2e0fd068 100644 --- a/packages/cache/__tests__/restoreCache.test.ts +++ b/packages/cache/__tests__/restoreCache.test.ts @@ -12,6 +12,12 @@ jest.mock('../src/internal/cacheUtils') jest.mock('../src/internal/tar') beforeAll(() => { + jest.spyOn(console, 'log').mockImplementation(() => {}) + jest.spyOn(core, 'debug').mockImplementation(() => {}) + jest.spyOn(core, 'info').mockImplementation(() => {}) + jest.spyOn(core, 'warning').mockImplementation(() => {}) + jest.spyOn(core, 'error').mockImplementation(() => {}) + // eslint-disable-next-line @typescript-eslint/promise-function-async jest.spyOn(cacheUtils, 'getCacheFileName').mockImplementation(cm => { const actualUtils = jest.requireActual('../src/internal/cacheUtils') @@ -56,7 +62,6 @@ test('restore with no cache found', async () => { const paths = ['node_modules'] const key = 'node-test' - const infoMock = jest.spyOn(core, 'info') jest.spyOn(cacheHttpClient, 'getCacheEntry').mockImplementation(async () => { return Promise.resolve(null) }) @@ -64,9 +69,6 @@ test('restore with no cache found', async () => { const cacheKey = await restoreCache(paths, key) expect(cacheKey).toBe(undefined) - expect(infoMock).toHaveBeenCalledWith( - `Cache not found for input keys: ${key}` - ) }) test('restore with server error should fail', async () => { @@ -87,8 +89,6 @@ test('restore with restore keys and no cache found', async () => { const key = 'node-test' const restoreKey = 'node-' - const infoMock = jest.spyOn(core, 'info') - jest.spyOn(cacheHttpClient, 'getCacheEntry').mockImplementation(async () => { return Promise.resolve(null) }) @@ -96,17 +96,12 @@ test('restore with restore keys and no cache found', async () => { const cacheKey = await restoreCache(paths, key, [restoreKey]) expect(cacheKey).toBe(undefined) - expect(infoMock).toHaveBeenCalledWith( - `Cache not found for input keys: ${key}, ${restoreKey}` - ) }) test('restore with gzip compressed cache found', async () => { const paths = ['node_modules'] const key = 'node-test' - const infoMock = jest.spyOn(core, 'info') - const cacheEntry: ArtifactCacheEntry = { cacheKey: key, scope: 'refs/heads/master', @@ -128,8 +123,8 @@ test('restore with gzip compressed cache found', async () => { const downloadCacheMock = jest.spyOn(cacheHttpClient, 'downloadCache') const fileSize = 142 - const getArchiveFileSizeMock = jest - .spyOn(cacheUtils, 'getArchiveFileSize') + const getArchiveFileSizeIsBytesMock = jest + .spyOn(cacheUtils, 'getArchiveFileSizeIsBytes') .mockReturnValue(fileSize) const extractTarMock = jest.spyOn(tar, 'extractTar') @@ -151,7 +146,7 @@ test('restore with gzip compressed cache found', async () => { cacheEntry.archiveLocation, archivePath ) - expect(getArchiveFileSizeMock).toHaveBeenCalledWith(archivePath) + expect(getArchiveFileSizeIsBytesMock).toHaveBeenCalledWith(archivePath) expect(extractTarMock).toHaveBeenCalledTimes(1) expect(extractTarMock).toHaveBeenCalledWith(archivePath, compression) @@ -159,11 +154,10 @@ test('restore with gzip compressed cache found', async () => { expect(unlinkFileMock).toHaveBeenCalledTimes(1) expect(unlinkFileMock).toHaveBeenCalledWith(archivePath) - expect(infoMock).toHaveBeenCalledWith(`Cache restored from key: ${key}`) expect(getCompressionMock).toHaveBeenCalledTimes(1) }) -test('restore with a pull request event and zstd compressed cache found', async () => { +test('restore with zstd compressed cache found', async () => { const paths = ['node_modules'] const key = 'node-test' @@ -189,8 +183,8 @@ test('restore with a pull request event and zstd compressed cache found', async const downloadCacheMock = jest.spyOn(cacheHttpClient, 'downloadCache') const fileSize = 62915000 - const getArchiveFileSizeMock = jest - .spyOn(cacheUtils, 'getArchiveFileSize') + const getArchiveFileSizeIsBytesMock = jest + .spyOn(cacheUtils, 'getArchiveFileSizeIsBytes') .mockReturnValue(fileSize) const extractTarMock = jest.spyOn(tar, 'extractTar') @@ -210,13 +204,11 @@ test('restore with a pull request event and zstd compressed cache found', async cacheEntry.archiveLocation, archivePath ) - expect(getArchiveFileSizeMock).toHaveBeenCalledWith(archivePath) + expect(getArchiveFileSizeIsBytesMock).toHaveBeenCalledWith(archivePath) expect(infoMock).toHaveBeenCalledWith(`Cache Size: ~60 MB (62915000 B)`) expect(extractTarMock).toHaveBeenCalledTimes(1) expect(extractTarMock).toHaveBeenCalledWith(archivePath, compression) - - expect(infoMock).toHaveBeenCalledWith(`Cache restored from key: ${key}`) expect(getCompressionMock).toHaveBeenCalledTimes(1) }) @@ -247,8 +239,8 @@ test('restore with cache found for restore key', async () => { const downloadCacheMock = jest.spyOn(cacheHttpClient, 'downloadCache') const fileSize = 142 - const getArchiveFileSizeMock = jest - .spyOn(cacheUtils, 'getArchiveFileSize') + const getArchiveFileSizeIsBytesMock = jest + .spyOn(cacheUtils, 'getArchiveFileSizeIsBytes') .mockReturnValue(fileSize) const extractTarMock = jest.spyOn(tar, 'extractTar') @@ -268,14 +260,10 @@ test('restore with cache found for restore key', async () => { cacheEntry.archiveLocation, archivePath ) - expect(getArchiveFileSizeMock).toHaveBeenCalledWith(archivePath) + expect(getArchiveFileSizeIsBytesMock).toHaveBeenCalledWith(archivePath) expect(infoMock).toHaveBeenCalledWith(`Cache Size: ~0 MB (142 B)`) expect(extractTarMock).toHaveBeenCalledTimes(1) expect(extractTarMock).toHaveBeenCalledWith(archivePath, compression) - - expect(infoMock).toHaveBeenCalledWith( - `Cache restored from key: ${restoreKey}` - ) expect(getCompressionMock).toHaveBeenCalledTimes(1) }) diff --git a/packages/cache/__tests__/saveCache.test.ts b/packages/cache/__tests__/saveCache.test.ts index 720d2ad6..71132878 100644 --- a/packages/cache/__tests__/saveCache.test.ts +++ b/packages/cache/__tests__/saveCache.test.ts @@ -1,3 +1,4 @@ +import * as core from '@actions/core' import * as path from 'path' import {saveCache} from '../src/cache' import * as cacheHttpClient from '../src/internal/cacheHttpClient' @@ -5,12 +6,17 @@ import * as cacheUtils from '../src/internal/cacheUtils' import {CacheFilename, CompressionMethod} from '../src/internal/constants' import * as tar from '../src/internal/tar' -jest.mock('@actions/core') jest.mock('../src/internal/cacheHttpClient') jest.mock('../src/internal/cacheUtils') jest.mock('../src/internal/tar') beforeAll(() => { + jest.spyOn(console, 'log').mockImplementation(() => {}) + jest.spyOn(core, 'debug').mockImplementation(() => {}) + jest.spyOn(core, 'info').mockImplementation(() => {}) + jest.spyOn(core, 'warning').mockImplementation(() => {}) + jest.spyOn(core, 'error').mockImplementation(() => {}) + // eslint-disable-next-line @typescript-eslint/promise-function-async jest.spyOn(cacheUtils, 'getCacheFileName').mockImplementation(cm => { const actualUtils = jest.requireActual('../src/internal/cacheUtils') @@ -42,7 +48,9 @@ test('save with large cache outputs should fail', async () => { const createTarMock = jest.spyOn(tar, 'createTar') const cacheSize = 6 * 1024 * 1024 * 1024 //~6GB, over the 5GB limit - jest.spyOn(cacheUtils, 'getArchiveFileSize').mockReturnValueOnce(cacheSize) + jest + .spyOn(cacheUtils, 'getArchiveFileSizeIsBytes') + .mockReturnValueOnce(cacheSize) const compression = CompressionMethod.Gzip const getCompressionMock = jest .spyOn(cacheUtils, 'getCompressionMethod') diff --git a/scripts/verify-cache-files.sh b/packages/cache/__tests__/verify-cache-files.sh similarity index 100% rename from scripts/verify-cache-files.sh rename to packages/cache/__tests__/verify-cache-files.sh diff --git a/packages/cache/package-lock.json b/packages/cache/package-lock.json index 9d8b6cbf..08f18a7c 100644 --- a/packages/cache/package-lock.json +++ b/packages/cache/package-lock.json @@ -1,6 +1,6 @@ { "name": "@actions/cache", - "version": "1.0.0", + "version": "0.1.0", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/packages/cache/package.json b/packages/cache/package.json index fea57f0b..69e93181 100644 --- a/packages/cache/package.json +++ b/packages/cache/package.json @@ -1,6 +1,6 @@ { "name": "@actions/cache", - "version": "1.0.0", + "version": "0.1.0", "preview": true, "description": "Actions cache lib", "keywords": [ diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index 5f55c82b..fc04a297 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -9,6 +9,7 @@ export class ValidationError extends Error { constructor(message: string) { super(message) this.name = 'ValidationError' + Object.setPrototypeOf(this, ValidationError.prototype) } } @@ -16,6 +17,7 @@ export class ReserveCacheError extends Error { constructor(message: string) { super(message) this.name = 'ReserveCacheError' + Object.setPrototypeOf(this, ReserveCacheError.prototype) } } @@ -47,7 +49,7 @@ function checkKey(key: string): void { * @param paths a list of file paths to restore from the cache * @param primaryKey an explicit key for restoring the cache * @param restoreKeys an optional ordered list of keys to use for restoring the cache if no cache hit occurred for key - * @returns string returns the key for the cache hit, otherwise return undefined + * @returns string returns the key for the cache hit, otherwise returns undefined */ export async function restoreCache( paths: string[], @@ -78,7 +80,7 @@ export async function restoreCache( compressionMethod }) if (!cacheEntry?.archiveLocation) { - core.info(`Cache not found for input keys: ${keys.join(', ')}`) + // Cache not found return undefined } @@ -92,7 +94,7 @@ export async function restoreCache( // Download the cache from the cache entry await cacheHttpClient.downloadCache(cacheEntry.archiveLocation, archivePath) - const archiveFileSize = utils.getArchiveFileSize(archivePath) + const archiveFileSize = utils.getArchiveFileSizeIsBytes(archivePath) core.info( `Cache Size: ~${Math.round( archiveFileSize / (1024 * 1024) @@ -109,8 +111,6 @@ export async function restoreCache( } } - core.info(`Cache restored from key: ${cacheEntry && cacheEntry.cacheKey}`) - return cacheEntry.cacheKey } @@ -120,7 +120,7 @@ export async function restoreCache( * @param paths a list of file paths to be cached * @param key an explicit key for restoring the cache * @param options cache upload options - * @returns number returns cacheId if the cache was saved successfully + * @returns number returns cacheId if the cache was saved successfully and throws an error if save fails */ export async function saveCache( paths: string[], @@ -158,7 +158,7 @@ export async function saveCache( await createTar(archiveFolder, cachePaths, compressionMethod) const fileSizeLimit = 5 * 1024 * 1024 * 1024 // 5GB per repo limit - const archiveFileSize = utils.getArchiveFileSize(archivePath) + const archiveFileSize = utils.getArchiveFileSizeIsBytes(archivePath) core.debug(`File Size: ${archiveFileSize}`) if (archiveFileSize > fileSizeLimit) { throw new Error( diff --git a/packages/cache/src/internal/cacheHttpClient.ts b/packages/cache/src/internal/cacheHttpClient.ts index 43692fa7..b7d34448 100644 --- a/packages/cache/src/internal/cacheHttpClient.ts +++ b/packages/cache/src/internal/cacheHttpClient.ts @@ -242,7 +242,7 @@ export async function downloadCache( if (contentLengthHeader) { const expectedLength = parseInt(contentLengthHeader) - const actualLength = utils.getArchiveFileSize(archivePath) + const actualLength = utils.getArchiveFileSizeIsBytes(archivePath) if (actualLength !== expectedLength) { throw new Error( @@ -399,7 +399,7 @@ export async function saveCache( // Commit Cache core.debug('Commiting cache') - const cacheSize = utils.getArchiveFileSize(archivePath) + const cacheSize = utils.getArchiveFileSizeIsBytes(archivePath) const commitCacheResponse = await commitCache(httpClient, cacheId, cacheSize) if (!isSuccessStatusCode(commitCacheResponse.statusCode)) { throw new Error( diff --git a/packages/cache/src/internal/cacheUtils.ts b/packages/cache/src/internal/cacheUtils.ts index 8743963a..f3f85006 100644 --- a/packages/cache/src/internal/cacheUtils.ts +++ b/packages/cache/src/internal/cacheUtils.ts @@ -34,7 +34,7 @@ export async function createTempDirectory(): Promise { return dest } -export function getArchiveFileSize(filePath: string): number { +export function getArchiveFileSizeIsBytes(filePath: string): number { return fs.statSync(filePath).size } @@ -80,6 +80,7 @@ async function getVersion(app: string): Promise { return versionOutput } +// Use zstandard if possible to maximize cache performance export async function getCompressionMethod(): Promise { const versionOutput = await getVersion('zstd') return versionOutput.toLowerCase().includes('zstd command line interface') diff --git a/packages/cache/src/options.ts b/packages/cache/src/options.ts index c5ecdad5..97441c1e 100644 --- a/packages/cache/src/options.ts +++ b/packages/cache/src/options.ts @@ -9,7 +9,7 @@ export interface UploadOptions { */ uploadConcurrency?: number /** - * Maximum chunk size for cache upload + * Maximum chunk size in bytes for cache upload * * @default 32MB */