Closed cdce8p closed 6 months ago
/CC @bethanyj28 Would you mind taking a look at this one? It fixes a bug which was just recently introduced.
@cdce8p - thanks for the contribution! This seems reasonable to me. Could you add a test case in https://github.com/actions/cache/blob/main/__tests__/restoreImpl.test.ts to validate? Thank you!
This seems reasonable to me. Could you add a test case in https://github.com/actions/cache/blob/main/__tests__/restoreImpl.test.ts to validate? Thank you!
I thought about that but I'm not really sure how to do it. The only thing not tested is process.exit(1);
however from what I've read online it's difficult to test that with jest
. Tbh I wouldn't consider myself an expert in that area. If you've any pointers for me that would be appreciated.
I already looked at #1217 which first introduced the process.exit
but AFAICT it didn't add any tests either.
👋🏻 @cdce8p - sorry for the delay! While the initial change might've not had tests, it would be really nice to validate in the future that we don't regress here. I believe mocking process.exit
isn't too bad in jest. Could you do something like this?
test("restore failure with earlyExit should call process exit", async () => {
// mock `process.exit`
const processExitMock = jest.spyOn(process, "exit").mockImplementation();
// simulate a failure
testUtils.setInput(Inputs.Path, "node_modules");
const failedMock = jest.spyOn(core, "setFailed");
const restoreCacheMock = jest.spyOn(cache, "restoreCache");
// set true for `earlyExit`
await restoreImpl(new StateProvider(), true);
expect(restoreCacheMock).toHaveBeenCalledTimes(0);
expect(failedMock).toHaveBeenCalledWith(
"Input required and not supplied: key"
);
// validate `process.exit` called
expect(processExitMock).toHaveBeenCalledWith(1);
});
Thank you!
@bethanyj28 Would you mind tagging the 4.0.2
release? It would be awesome if this bug would finally be fixed.
I already updated the version in package.json
and the changelog in this PR.
Definitely! I appreciate you updating the version 🙏🏻
CACHING ACTION
Description
process.exit
overwrites the exit code set bycore.setFailed
, e.g. the workflow would continue even thoughfail-on-cache-miss
is set and no cache is found.Originally introduced in
1217 by @chkimes
/CC @kgrzebien
Motivation and Context
Fixes #1265
How Has This Been Tested?
Created a workflow which should restore a non-existing cache. With
v4.0.0
the workflow continues, with this PR it fails as expected (withfail-on-cache-miss
set).Screenshots (if appropriate):
Types of changes
Checklist: