From 91d7bd61be3bd79dd6b245e613b5d4c9cd98ac58 Mon Sep 17 00:00:00 2001 From: Sankalp Kotewar <98868223+kotewar@users.noreply.github.com> Date: Mon, 12 Dec 2022 07:26:18 +0000 Subject: [PATCH] Fixed review comments and tests --- __tests__/restoreOnly.test.ts | 43 ++++++++++++++++++--------------- __tests__/saveImpl.test.ts | 4 +-- __tests__/saveOnly.test.ts | 4 +++ __tests__/stateProvider.test.ts | 20 +++++++++++++++ dist/restore-only/index.js | 12 ++++----- dist/restore/index.js | 12 ++++----- dist/save-only/index.js | 20 ++++++--------- dist/save/index.js | 20 ++++++--------- src/constants.ts | 5 ---- src/saveImpl.ts | 8 ++---- 10 files changed, 76 insertions(+), 72 deletions(-) create mode 100644 __tests__/stateProvider.test.ts diff --git a/__tests__/restoreOnly.test.ts b/__tests__/restoreOnly.test.ts index 4812f02..fa9d685 100644 --- a/__tests__/restoreOnly.test.ts +++ b/__tests__/restoreOnly.test.ts @@ -2,8 +2,7 @@ import * as cache from "@actions/cache"; import * as core from "@actions/core"; import { Events, RefKey } from "../src/constants"; -import run from "../src/restoreImpl"; -import { StateProvider } from "../src/stateProvider"; +import run from "../src/restoreOnly"; import * as actionUtils from "../src/utils/actionUtils"; import * as testUtils from "../src/utils/testUtils"; @@ -56,19 +55,20 @@ test("restore with no cache found", async () => { const infoMock = jest.spyOn(core, "info"); const failedMock = jest.spyOn(core, "setFailed"); - const stateMock = jest.spyOn(core, "saveState"); + const outputMock = jest.spyOn(core, "setOutput"); const restoreCacheMock = jest .spyOn(cache, "restoreCache") .mockImplementationOnce(() => { return Promise.resolve(undefined); }); - await run(new StateProvider()); + await run(); expect(restoreCacheMock).toHaveBeenCalledTimes(1); expect(restoreCacheMock).toHaveBeenCalledWith([path], key, []); - expect(stateMock).toHaveBeenCalledWith("CACHE_KEY", key); + expect(outputMock).toHaveBeenCalledWith("cache-primary-key", key); + expect(outputMock).toHaveBeenCalledTimes(1); expect(failedMock).toHaveBeenCalledTimes(0); expect(infoMock).toHaveBeenCalledWith( @@ -88,19 +88,19 @@ test("restore with restore keys and no cache found", async () => { const infoMock = jest.spyOn(core, "info"); const failedMock = jest.spyOn(core, "setFailed"); - const stateMock = jest.spyOn(core, "saveState"); + const outputMock = jest.spyOn(core, "setOutput"); const restoreCacheMock = jest .spyOn(cache, "restoreCache") .mockImplementationOnce(() => { return Promise.resolve(undefined); }); - await run(new StateProvider()); + await run(); expect(restoreCacheMock).toHaveBeenCalledTimes(1); expect(restoreCacheMock).toHaveBeenCalledWith([path], key, [restoreKey]); - expect(stateMock).toHaveBeenCalledWith("CACHE_KEY", key); + expect(outputMock).toHaveBeenCalledWith("cache-primary-key", key); expect(failedMock).toHaveBeenCalledTimes(0); expect(infoMock).toHaveBeenCalledWith( @@ -118,22 +118,23 @@ test("restore with cache found for key", async () => { const infoMock = jest.spyOn(core, "info"); const failedMock = jest.spyOn(core, "setFailed"); - const stateMock = jest.spyOn(core, "saveState"); - const setCacheHitOutputMock = jest.spyOn(core, "setOutput"); + const outputMock = jest.spyOn(core, "setOutput"); const restoreCacheMock = jest .spyOn(cache, "restoreCache") .mockImplementationOnce(() => { return Promise.resolve(key); }); - await run(new StateProvider()); + await run(); expect(restoreCacheMock).toHaveBeenCalledTimes(1); expect(restoreCacheMock).toHaveBeenCalledWith([path], key, []); - expect(stateMock).toHaveBeenCalledWith("CACHE_KEY", key); - expect(setCacheHitOutputMock).toHaveBeenCalledTimes(1); - expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "true"); + expect(outputMock).toHaveBeenCalledWith("cache-primary-key", key); + expect(outputMock).toHaveBeenCalledWith("cache-hit", "true"); + expect(outputMock).toHaveBeenCalledWith("cache-restore-key", key); + + expect(outputMock).toHaveBeenCalledTimes(3); expect(infoMock).toHaveBeenCalledWith(`Cache restored from key: ${key}`); expect(failedMock).toHaveBeenCalledTimes(0); @@ -151,22 +152,24 @@ test("restore with cache found for restore key", async () => { const infoMock = jest.spyOn(core, "info"); const failedMock = jest.spyOn(core, "setFailed"); - const stateMock = jest.spyOn(core, "saveState"); - const setCacheHitOutputMock = jest.spyOn(core, "setOutput"); + const outputMock = jest.spyOn(core, "setOutput"); const restoreCacheMock = jest .spyOn(cache, "restoreCache") .mockImplementationOnce(() => { return Promise.resolve(restoreKey); }); - await run(new StateProvider()); + await run(); expect(restoreCacheMock).toHaveBeenCalledTimes(1); expect(restoreCacheMock).toHaveBeenCalledWith([path], key, [restoreKey]); - expect(stateMock).toHaveBeenCalledWith("CACHE_KEY", key); - expect(setCacheHitOutputMock).toHaveBeenCalledTimes(1); - expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "false"); + expect(outputMock).toHaveBeenCalledWith("cache-primary-key", key); + expect(outputMock).toHaveBeenCalledWith("cache-hit", "false"); + expect(outputMock).toHaveBeenCalledWith("cache-restore-key", restoreKey); + + expect(outputMock).toHaveBeenCalledTimes(3); + expect(infoMock).toHaveBeenCalledWith( `Cache restored from key: ${restoreKey}` ); diff --git a/__tests__/saveImpl.test.ts b/__tests__/saveImpl.test.ts index 78e964a..ab43047 100644 --- a/__tests__/saveImpl.test.ts +++ b/__tests__/saveImpl.test.ts @@ -94,9 +94,7 @@ test("save with no primary key in state outputs warning", async () => { await run(new StateProvider()); expect(saveCacheMock).toHaveBeenCalledTimes(0); - expect(logWarningMock).toHaveBeenCalledWith( - `Error retrieving key from state.` - ); + expect(logWarningMock).toHaveBeenCalledWith(`Key is not specified.`); expect(logWarningMock).toHaveBeenCalledTimes(1); expect(failedMock).toHaveBeenCalledTimes(0); }); diff --git a/__tests__/saveOnly.test.ts b/__tests__/saveOnly.test.ts index 5604e3a..2c2e0b8 100644 --- a/__tests__/saveOnly.test.ts +++ b/__tests__/saveOnly.test.ts @@ -15,6 +15,10 @@ beforeAll(() => { return jest.requireActual("@actions/core").getInput(name, options); }); + jest.spyOn(core, "setOutput").mockImplementation((key, value) => { + return jest.requireActual("@actions/core").getInput(key, value); + }); + jest.spyOn(actionUtils, "getInputAsArray").mockImplementation( (name, options) => { return jest diff --git a/__tests__/stateProvider.test.ts b/__tests__/stateProvider.test.ts new file mode 100644 index 0000000..abbc7f3 --- /dev/null +++ b/__tests__/stateProvider.test.ts @@ -0,0 +1,20 @@ +import * as cache from "@actions/cache"; +import * as core from "@actions/core"; + +import { Events, RefKey } from "../src/constants"; +import * as actionUtils from "../src/utils/actionUtils"; +import * as testUtils from "../src/utils/testUtils"; + +jest.mock("@actions/core"); +jest.mock("@actions/cache"); + +beforeAll(() => { + jest.spyOn(core, "getInput").mockImplementation((name, options) => { + return jest.requireActual("@actions/core").getInput(name, options); + }); +}); + +afterEach(() => { + delete process.env[Events.Key]; + delete process.env[RefKey]; +}); diff --git a/dist/restore-only/index.js b/dist/restore-only/index.js index 961c8a6..0192c06 100644 --- a/dist/restore-only/index.js +++ b/dist/restore-only/index.js @@ -4943,7 +4943,7 @@ exports.checkBypass = checkBypass; "use strict"; Object.defineProperty(exports, "__esModule", { value: true }); -exports.stateToOutputMap = exports.RefKey = exports.Events = exports.State = exports.Outputs = exports.Inputs = void 0; +exports.RefKey = exports.Events = exports.State = exports.Outputs = exports.Inputs = void 0; var Inputs; (function (Inputs) { Inputs["Key"] = "key"; @@ -4969,10 +4969,6 @@ var Events; Events["PullRequest"] = "pull_request"; })(Events = exports.Events || (exports.Events = {})); exports.RefKey = "GITHUB_REF"; -exports.stateToOutputMap = new Map([ - [State.CacheMatchedKey, Outputs.CacheRestoreKey], - [State.CachePrimaryKey, Outputs.CachePrimaryKey] -]); /***/ }), @@ -9388,8 +9384,12 @@ exports.StateProvider = StateProvider; class NullStateProvider extends StateProviderBase { constructor() { super(...arguments); + this.stateToOutputMap = new Map([ + [constants_1.State.CacheMatchedKey, constants_1.Outputs.CacheRestoreKey], + [constants_1.State.CachePrimaryKey, constants_1.Outputs.CachePrimaryKey] + ]); this.setState = (key, value) => { - core.setOutput(constants_1.stateToOutputMap.get(key), value); + core.setOutput(this.stateToOutputMap.get(key), value); }; // eslint-disable-next-line @typescript-eslint/no-unused-vars this.getState = (key) => ""; diff --git a/dist/restore/index.js b/dist/restore/index.js index af749dc..8638fbe 100644 --- a/dist/restore/index.js +++ b/dist/restore/index.js @@ -4943,7 +4943,7 @@ exports.checkBypass = checkBypass; "use strict"; Object.defineProperty(exports, "__esModule", { value: true }); -exports.stateToOutputMap = exports.RefKey = exports.Events = exports.State = exports.Outputs = exports.Inputs = void 0; +exports.RefKey = exports.Events = exports.State = exports.Outputs = exports.Inputs = void 0; var Inputs; (function (Inputs) { Inputs["Key"] = "key"; @@ -4969,10 +4969,6 @@ var Events; Events["PullRequest"] = "pull_request"; })(Events = exports.Events || (exports.Events = {})); exports.RefKey = "GITHUB_REF"; -exports.stateToOutputMap = new Map([ - [State.CacheMatchedKey, Outputs.CacheRestoreKey], - [State.CachePrimaryKey, Outputs.CachePrimaryKey] -]); /***/ }), @@ -9388,8 +9384,12 @@ exports.StateProvider = StateProvider; class NullStateProvider extends StateProviderBase { constructor() { super(...arguments); + this.stateToOutputMap = new Map([ + [constants_1.State.CacheMatchedKey, constants_1.Outputs.CacheRestoreKey], + [constants_1.State.CachePrimaryKey, constants_1.Outputs.CachePrimaryKey] + ]); this.setState = (key, value) => { - core.setOutput(constants_1.stateToOutputMap.get(key), value); + core.setOutput(this.stateToOutputMap.get(key), value); }; // eslint-disable-next-line @typescript-eslint/no-unused-vars this.getState = (key) => ""; diff --git a/dist/save-only/index.js b/dist/save-only/index.js index 96dd341..f417c7a 100644 --- a/dist/save-only/index.js +++ b/dist/save-only/index.js @@ -4972,7 +4972,7 @@ exports.checkBypass = checkBypass; "use strict"; Object.defineProperty(exports, "__esModule", { value: true }); -exports.stateToOutputMap = exports.RefKey = exports.Events = exports.State = exports.Outputs = exports.Inputs = void 0; +exports.RefKey = exports.Events = exports.State = exports.Outputs = exports.Inputs = void 0; var Inputs; (function (Inputs) { Inputs["Key"] = "key"; @@ -4998,10 +4998,6 @@ var Events; Events["PullRequest"] = "pull_request"; })(Events = exports.Events || (exports.Events = {})); exports.RefKey = "GITHUB_REF"; -exports.stateToOutputMap = new Map([ - [State.CacheMatchedKey, Outputs.CacheRestoreKey], - [State.CachePrimaryKey, Outputs.CachePrimaryKey] -]); /***/ }), @@ -9417,8 +9413,12 @@ exports.StateProvider = StateProvider; class NullStateProvider extends StateProviderBase { constructor() { super(...arguments); + this.stateToOutputMap = new Map([ + [constants_1.State.CacheMatchedKey, constants_1.Outputs.CacheRestoreKey], + [constants_1.State.CachePrimaryKey, constants_1.Outputs.CachePrimaryKey] + ]); this.setState = (key, value) => { - core.setOutput(constants_1.stateToOutputMap.get(key), value); + core.setOutput(this.stateToOutputMap.get(key), value); }; // eslint-disable-next-line @typescript-eslint/no-unused-vars this.getState = (key) => ""; @@ -41081,7 +41081,6 @@ Object.defineProperty(exports, "__esModule", { value: true }); const cache = __importStar(__webpack_require__(692)); const core = __importStar(__webpack_require__(470)); const constants_1 = __webpack_require__(196); -const stateProvider_1 = __webpack_require__(309); const utils = __importStar(__webpack_require__(443)); // Catch and log any unhandled exceptions. These exceptions can leak out of the uploadChunk method in // @actions/toolkit when a failed upload closes the file descriptor causing any in-process reads to @@ -41102,12 +41101,7 @@ function saveImpl(stateProvider) { const primaryKey = stateProvider.getState(constants_1.State.CachePrimaryKey) || core.getInput(constants_1.Inputs.Key); if (!primaryKey) { - if (stateProvider instanceof stateProvider_1.StateProvider) { - utils.logWarning(`Error retrieving key from state.`); - } - else { - utils.logWarning(`Error retrieving key from input.`); - } + utils.logWarning(`Key is not specified.`); return; } // If matched restore key is same as primary key, then do not save cache diff --git a/dist/save/index.js b/dist/save/index.js index 3032065..c11e5fb 100644 --- a/dist/save/index.js +++ b/dist/save/index.js @@ -4943,7 +4943,7 @@ exports.checkBypass = checkBypass; "use strict"; Object.defineProperty(exports, "__esModule", { value: true }); -exports.stateToOutputMap = exports.RefKey = exports.Events = exports.State = exports.Outputs = exports.Inputs = void 0; +exports.RefKey = exports.Events = exports.State = exports.Outputs = exports.Inputs = void 0; var Inputs; (function (Inputs) { Inputs["Key"] = "key"; @@ -4969,10 +4969,6 @@ var Events; Events["PullRequest"] = "pull_request"; })(Events = exports.Events || (exports.Events = {})); exports.RefKey = "GITHUB_REF"; -exports.stateToOutputMap = new Map([ - [State.CacheMatchedKey, Outputs.CacheRestoreKey], - [State.CachePrimaryKey, Outputs.CachePrimaryKey] -]); /***/ }), @@ -9388,8 +9384,12 @@ exports.StateProvider = StateProvider; class NullStateProvider extends StateProviderBase { constructor() { super(...arguments); + this.stateToOutputMap = new Map([ + [constants_1.State.CacheMatchedKey, constants_1.Outputs.CacheRestoreKey], + [constants_1.State.CachePrimaryKey, constants_1.Outputs.CachePrimaryKey] + ]); this.setState = (key, value) => { - core.setOutput(constants_1.stateToOutputMap.get(key), value); + core.setOutput(this.stateToOutputMap.get(key), value); }; // eslint-disable-next-line @typescript-eslint/no-unused-vars this.getState = (key) => ""; @@ -41052,7 +41052,6 @@ Object.defineProperty(exports, "__esModule", { value: true }); const cache = __importStar(__webpack_require__(692)); const core = __importStar(__webpack_require__(470)); const constants_1 = __webpack_require__(196); -const stateProvider_1 = __webpack_require__(309); const utils = __importStar(__webpack_require__(443)); // Catch and log any unhandled exceptions. These exceptions can leak out of the uploadChunk method in // @actions/toolkit when a failed upload closes the file descriptor causing any in-process reads to @@ -41073,12 +41072,7 @@ function saveImpl(stateProvider) { const primaryKey = stateProvider.getState(constants_1.State.CachePrimaryKey) || core.getInput(constants_1.Inputs.Key); if (!primaryKey) { - if (stateProvider instanceof stateProvider_1.StateProvider) { - utils.logWarning(`Error retrieving key from state.`); - } - else { - utils.logWarning(`Error retrieving key from input.`); - } + utils.logWarning(`Key is not specified.`); return; } // If matched restore key is same as primary key, then do not save cache diff --git a/src/constants.ts b/src/constants.ts index 091b2a1..0c5b610 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -23,8 +23,3 @@ export enum Events { } export const RefKey = "GITHUB_REF"; - -export const stateToOutputMap = new Map([ - [State.CacheMatchedKey, Outputs.CacheRestoreKey], - [State.CachePrimaryKey, Outputs.CachePrimaryKey] -]); diff --git a/src/saveImpl.ts b/src/saveImpl.ts index 4c02c24..1f0d366 100644 --- a/src/saveImpl.ts +++ b/src/saveImpl.ts @@ -2,7 +2,7 @@ import * as cache from "@actions/cache"; import * as core from "@actions/core"; import { Events, Inputs, State } from "./constants"; -import { IStateProvider, StateProvider } from "./stateProvider"; +import { IStateProvider } from "./stateProvider"; import * as utils from "./utils/actionUtils"; // Catch and log any unhandled exceptions. These exceptions can leak out of the uploadChunk method in @@ -32,11 +32,7 @@ async function saveImpl(stateProvider: IStateProvider): Promise { core.getInput(Inputs.Key); if (!primaryKey) { - if (stateProvider instanceof StateProvider) { - utils.logWarning(`Error retrieving key from state.`); - } else { - utils.logWarning(`Error retrieving key from input.`); - } + utils.logWarning(`Key is not specified.`); return; }