Fix fail-on-cache-miss not working

[cdce8p]

Description

process.exit overwrites the exit code set by core.setFailed, e.g. the workflow would continue even though fail-on-cache-miss is set and no cache is found.

Originally introduced in

• Consume latest toolkit and fix dangling promise bug #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 (with fail-on-cache-miss set).

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (add or update README or docs)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[cdce8p]

/CC @bethanyj28 Would you mind taking a look at this one? It fixes a bug which was just recently introduced.

[bethanyj28]

@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!

[cdce8p]

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.

[bethanyj28]

👋🏻 @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]

Thank you!

[cdce8p]

@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.

[bethanyj28]

Definitely! I appreciate you updating the version 🙏🏻

[Emmanuelruina]

CACHING ACTION