From 95c17983692a3b9f2b3b50b7372247f6fe140cec Mon Sep 17 00:00:00 2001 From: Josh Gross Date: Thu, 21 Nov 2019 14:37:54 -0500 Subject: [PATCH] Remove validation failures and warning annotations (#108) * Update warnings behavior * Add void return type --- __tests__/actionUtils.test.ts | 10 ++++++ __tests__/restore.test.ts | 22 +++++--------- __tests__/save.test.ts | 57 ++++++++++++++++++++++++----------- package-lock.json | 2 +- package.json | 2 +- src/restore.ts | 5 +-- src/save.ts | 19 +++++++++--- src/utils/actionUtils.ts | 5 +++ 8 files changed, 82 insertions(+), 40 deletions(-) diff --git a/__tests__/actionUtils.test.ts b/__tests__/actionUtils.test.ts index 4688b5d..f46d65d 100644 --- a/__tests__/actionUtils.test.ts +++ b/__tests__/actionUtils.test.ts @@ -162,6 +162,16 @@ test("getCacheState with valid state", () => { expect(getStateMock).toHaveBeenCalledTimes(1); }); +test("logWarning logs a message with a warning prefix", () => { + const message = "A warning occurred."; + + const infoMock = jest.spyOn(core, "info"); + + actionUtils.logWarning(message); + + expect(infoMock).toHaveBeenCalledWith(`[warning]${message}`); +}); + test("isValidEvent returns false for unknown event", () => { const event = "foo"; process.env[Events.Key] = event; diff --git a/__tests__/restore.test.ts b/__tests__/restore.test.ts index 1919e30..78b00ec 100644 --- a/__tests__/restore.test.ts +++ b/__tests__/restore.test.ts @@ -50,14 +50,16 @@ afterEach(() => { delete process.env[Events.Key]; }); -test("restore with invalid event", async () => { +test("restore with invalid event outputs warning", async () => { + const logWarningMock = jest.spyOn(actionUtils, "logWarning"); const failedMock = jest.spyOn(core, "setFailed"); const invalidEvent = "commit_comment"; process.env[Events.Key] = invalidEvent; await run(); - expect(failedMock).toHaveBeenCalledWith( + expect(logWarningMock).toHaveBeenCalledWith( `Event Validation Error: The event type ${invalidEvent} is not supported. Only push, pull_request events are supported at this time.` ); + expect(failedMock).toHaveBeenCalledTimes(0); }); test("restore with no path should fail", async () => { @@ -126,7 +128,6 @@ test("restore with no cache found", async () => { }); const infoMock = jest.spyOn(core, "info"); - const warningMock = jest.spyOn(core, "warning"); const failedMock = jest.spyOn(core, "setFailed"); const stateMock = jest.spyOn(core, "saveState"); @@ -138,7 +139,6 @@ test("restore with no cache found", async () => { await run(); expect(stateMock).toHaveBeenCalledWith("CACHE_KEY", key); - expect(warningMock).toHaveBeenCalledTimes(0); expect(failedMock).toHaveBeenCalledTimes(0); expect(infoMock).toHaveBeenCalledWith( @@ -153,7 +153,7 @@ test("restore with server error should fail", async () => { key }); - const warningMock = jest.spyOn(core, "warning"); + const logWarningMock = jest.spyOn(actionUtils, "logWarning"); const failedMock = jest.spyOn(core, "setFailed"); const stateMock = jest.spyOn(core, "saveState"); @@ -168,8 +168,8 @@ test("restore with server error should fail", async () => { expect(stateMock).toHaveBeenCalledWith("CACHE_KEY", key); - expect(warningMock).toHaveBeenCalledTimes(1); - expect(warningMock).toHaveBeenCalledWith("HTTP Error Occurred"); + expect(logWarningMock).toHaveBeenCalledTimes(1); + expect(logWarningMock).toHaveBeenCalledWith("HTTP Error Occurred"); expect(setCacheHitOutputMock).toHaveBeenCalledTimes(1); expect(setCacheHitOutputMock).toHaveBeenCalledWith(false); @@ -187,7 +187,6 @@ test("restore with restore keys and no cache found", async () => { }); const infoMock = jest.spyOn(core, "info"); - const warningMock = jest.spyOn(core, "warning"); const failedMock = jest.spyOn(core, "setFailed"); const stateMock = jest.spyOn(core, "saveState"); @@ -199,7 +198,6 @@ test("restore with restore keys and no cache found", async () => { await run(); expect(stateMock).toHaveBeenCalledWith("CACHE_KEY", key); - expect(warningMock).toHaveBeenCalledTimes(0); expect(failedMock).toHaveBeenCalledTimes(0); expect(infoMock).toHaveBeenCalledWith( @@ -216,7 +214,6 @@ test("restore with cache found", async () => { }); const infoMock = jest.spyOn(core, "info"); - const warningMock = jest.spyOn(core, "warning"); const failedMock = jest.spyOn(core, "setFailed"); const stateMock = jest.spyOn(core, "saveState"); @@ -281,7 +278,6 @@ test("restore with cache found", async () => { expect(setCacheHitOutputMock).toHaveBeenCalledWith(true); expect(infoMock).toHaveBeenCalledWith(`Cache restored from key: ${key}`); - expect(warningMock).toHaveBeenCalledTimes(0); expect(failedMock).toHaveBeenCalledTimes(0); }); @@ -296,7 +292,6 @@ test("restore with a pull request event and cache found", async () => { process.env[Events.Key] = Events.PullRequest; const infoMock = jest.spyOn(core, "info"); - const warningMock = jest.spyOn(core, "warning"); const failedMock = jest.spyOn(core, "setFailed"); const stateMock = jest.spyOn(core, "saveState"); @@ -362,7 +357,6 @@ test("restore with a pull request event and cache found", async () => { expect(setCacheHitOutputMock).toHaveBeenCalledWith(true); expect(infoMock).toHaveBeenCalledWith(`Cache restored from key: ${key}`); - expect(warningMock).toHaveBeenCalledTimes(0); expect(failedMock).toHaveBeenCalledTimes(0); }); @@ -377,7 +371,6 @@ test("restore with cache found for restore key", async () => { }); const infoMock = jest.spyOn(core, "info"); - const warningMock = jest.spyOn(core, "warning"); const failedMock = jest.spyOn(core, "setFailed"); const stateMock = jest.spyOn(core, "saveState"); @@ -445,6 +438,5 @@ test("restore with cache found for restore key", async () => { expect(infoMock).toHaveBeenCalledWith( `Cache restored from key: ${restoreKey}` ); - expect(warningMock).toHaveBeenCalledTimes(0); expect(failedMock).toHaveBeenCalledTimes(0); }); diff --git a/__tests__/save.test.ts b/__tests__/save.test.ts index 67b13d2..89657c4 100644 --- a/__tests__/save.test.ts +++ b/__tests__/save.test.ts @@ -3,7 +3,7 @@ import * as exec from "@actions/exec"; import * as io from "@actions/io"; import * as path from "path"; import * as cacheHttpClient from "../src/cacheHttpClient"; -import { Inputs } from "../src/constants"; +import { Events, Inputs } from "../src/constants"; import { ArtifactCacheEntry } from "../src/contracts"; import run from "../src/save"; import * as actionUtils from "../src/utils/actionUtils"; @@ -32,6 +32,16 @@ beforeAll(() => { } ); + jest.spyOn(actionUtils, "isValidEvent").mockImplementation(() => { + const actualUtils = jest.requireActual("../src/utils/actionUtils"); + return actualUtils.isValidEvent(); + }); + + jest.spyOn(actionUtils, "getSupportedEvents").mockImplementation(() => { + const actualUtils = jest.requireActual("../src/utils/actionUtils"); + return actualUtils.getSupportedEvents(); + }); + jest.spyOn(actionUtils, "resolvePath").mockImplementation(filePath => { return path.resolve(filePath); }); @@ -45,12 +55,29 @@ beforeAll(() => { }); }); +beforeEach(() => { + process.env[Events.Key] = Events.Push; +}); + afterEach(() => { testUtils.clearInputs(); + delete process.env[Events.Key]; +}); + +test("save with invalid event outputs warning", async () => { + const logWarningMock = jest.spyOn(actionUtils, "logWarning"); + const failedMock = jest.spyOn(core, "setFailed"); + const invalidEvent = "commit_comment"; + process.env[Events.Key] = invalidEvent; + await run(); + expect(logWarningMock).toHaveBeenCalledWith( + `Event Validation Error: The event type ${invalidEvent} is not supported. Only push, pull_request events are supported at this time.` + ); + expect(failedMock).toHaveBeenCalledTimes(0); }); test("save with no primary key in state outputs warning", async () => { - const warningMock = jest.spyOn(core, "warning"); + const logWarningMock = jest.spyOn(actionUtils, "logWarning"); const failedMock = jest.spyOn(core, "setFailed"); const cacheEntry: ArtifactCacheEntry = { @@ -72,16 +99,15 @@ test("save with no primary key in state outputs warning", async () => { await run(); - expect(warningMock).toHaveBeenCalledWith( + expect(logWarningMock).toHaveBeenCalledWith( `Error retrieving key from state.` ); - expect(warningMock).toHaveBeenCalledTimes(1); + expect(logWarningMock).toHaveBeenCalledTimes(1); expect(failedMock).toHaveBeenCalledTimes(0); }); test("save with exact match returns early", async () => { const infoMock = jest.spyOn(core, "info"); - const warningMock = jest.spyOn(core, "warning"); const failedMock = jest.spyOn(core, "setFailed"); const primaryKey = "Linux-node-bb828da54c148048dd17899ba9fda624811cfb43"; @@ -112,12 +138,11 @@ test("save with exact match returns early", async () => { expect(execMock).toHaveBeenCalledTimes(0); - expect(warningMock).toHaveBeenCalledTimes(0); expect(failedMock).toHaveBeenCalledTimes(0); }); test("save with missing input outputs warning", async () => { - const warningMock = jest.spyOn(core, "warning"); + const logWarningMock = jest.spyOn(actionUtils, "logWarning"); const failedMock = jest.spyOn(core, "setFailed"); const primaryKey = "Linux-node-bb828da54c148048dd17899ba9fda624811cfb43"; @@ -140,15 +165,15 @@ test("save with missing input outputs warning", async () => { await run(); - expect(warningMock).toHaveBeenCalledWith( + expect(logWarningMock).toHaveBeenCalledWith( "Input required and not supplied: path" ); - expect(warningMock).toHaveBeenCalledTimes(1); + expect(logWarningMock).toHaveBeenCalledTimes(1); expect(failedMock).toHaveBeenCalledTimes(0); }); test("save with large cache outputs warning", async () => { - const warningMock = jest.spyOn(core, "warning"); + const logWarningMock = jest.spyOn(actionUtils, "logWarning"); const failedMock = jest.spyOn(core, "setFailed"); const primaryKey = "Linux-node-bb828da54c148048dd17899ba9fda624811cfb43"; @@ -200,8 +225,8 @@ test("save with large cache outputs warning", async () => { expect(execMock).toHaveBeenCalledTimes(1); expect(execMock).toHaveBeenCalledWith(`"tar"`, args); - expect(warningMock).toHaveBeenCalledTimes(1); - expect(warningMock).toHaveBeenCalledWith( + expect(logWarningMock).toHaveBeenCalledTimes(1); + expect(logWarningMock).toHaveBeenCalledWith( "Cache size of ~1024 MB (1073741824 B) is over the 400MB limit, not saving cache." ); @@ -209,7 +234,7 @@ test("save with large cache outputs warning", async () => { }); test("save with server error outputs warning", async () => { - const warningMock = jest.spyOn(core, "warning"); + const logWarningMock = jest.spyOn(actionUtils, "logWarning"); const failedMock = jest.spyOn(core, "setFailed"); const primaryKey = "Linux-node-bb828da54c148048dd17899ba9fda624811cfb43"; @@ -265,14 +290,13 @@ test("save with server error outputs warning", async () => { expect(saveCacheMock).toHaveBeenCalledTimes(1); expect(saveCacheMock).toHaveBeenCalledWith(primaryKey, archivePath); - expect(warningMock).toHaveBeenCalledTimes(1); - expect(warningMock).toHaveBeenCalledWith("HTTP Error Occurred"); + expect(logWarningMock).toHaveBeenCalledTimes(1); + expect(logWarningMock).toHaveBeenCalledWith("HTTP Error Occurred"); expect(failedMock).toHaveBeenCalledTimes(0); }); test("save with valid inputs uploads a cache", async () => { - const warningMock = jest.spyOn(core, "warning"); const failedMock = jest.spyOn(core, "setFailed"); const primaryKey = "Linux-node-bb828da54c148048dd17899ba9fda624811cfb43"; @@ -324,6 +348,5 @@ test("save with valid inputs uploads a cache", async () => { expect(saveCacheMock).toHaveBeenCalledTimes(1); expect(saveCacheMock).toHaveBeenCalledWith(primaryKey, archivePath); - expect(warningMock).toHaveBeenCalledTimes(0); expect(failedMock).toHaveBeenCalledTimes(0); }); diff --git a/package-lock.json b/package-lock.json index 9821cb1..2e8413e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "cache", - "version": "1.0.2", + "version": "1.0.3", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index dd09d47..42fbdbe 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "cache", - "version": "1.0.2", + "version": "1.0.3", "private": true, "description": "Cache dependencies and build outputs", "main": "dist/restore/index.js", diff --git a/src/restore.ts b/src/restore.ts index 87a2684..15570cd 100644 --- a/src/restore.ts +++ b/src/restore.ts @@ -10,13 +10,14 @@ async function run(): Promise { try { // Validate inputs, this can cause task failure if (!utils.isValidEvent()) { - core.setFailed( + utils.logWarning( `Event Validation Error: The event type ${ process.env[Events.Key] } is not supported. Only ${utils .getSupportedEvents() .join(", ")} events are supported at this time.` ); + return; } const cachePath = utils.resolvePath( @@ -118,7 +119,7 @@ async function run(): Promise { `Cache restored from key: ${cacheEntry && cacheEntry.cacheKey}` ); } catch (error) { - core.warning(error.message); + utils.logWarning(error.message); utils.setCacheHitOutput(false); } } catch (error) { diff --git a/src/save.ts b/src/save.ts index d660064..21f32d3 100644 --- a/src/save.ts +++ b/src/save.ts @@ -3,17 +3,28 @@ import { exec } from "@actions/exec"; import * as io from "@actions/io"; import * as path from "path"; import * as cacheHttpClient from "./cacheHttpClient"; -import { Inputs, State } from "./constants"; +import { Events, Inputs, State } from "./constants"; import * as utils from "./utils/actionUtils"; async function run(): Promise { try { + if (!utils.isValidEvent()) { + utils.logWarning( + `Event Validation Error: The event type ${ + process.env[Events.Key] + } is not supported. Only ${utils + .getSupportedEvents() + .join(", ")} events are supported at this time.` + ); + return; + } + const state = utils.getCacheState(); // Inputs are re-evaluted before the post action, so we want the original key used for restore const primaryKey = core.getState(State.CacheKey); if (!primaryKey) { - core.warning(`Error retrieving key from state.`); + utils.logWarning(`Error retrieving key from state.`); return; } @@ -58,7 +69,7 @@ async function run(): Promise { const archiveFileSize = utils.getArchiveFileSize(archivePath); core.debug(`File Size: ${archiveFileSize}`); if (archiveFileSize > fileSizeLimit) { - core.warning( + utils.logWarning( `Cache size of ~${Math.round( archiveFileSize / (1024 * 1024) )} MB (${archiveFileSize} B) is over the 400MB limit, not saving cache.` @@ -68,7 +79,7 @@ async function run(): Promise { await cacheHttpClient.saveCache(primaryKey, archivePath); } catch (error) { - core.warning(error.message); + utils.logWarning(error.message); } } diff --git a/src/utils/actionUtils.ts b/src/utils/actionUtils.ts index ba5b2ea..f6369fb 100644 --- a/src/utils/actionUtils.ts +++ b/src/utils/actionUtils.ts @@ -77,6 +77,11 @@ export function getCacheState(): ArtifactCacheEntry | undefined { return undefined; } +export function logWarning(message: string): void { + const warningPrefix = "[warning]"; + core.info(`${warningPrefix}${message}`); +} + export function resolvePath(filePath: string): string { if (filePath[0] === "~") { const home = os.homedir();