Fix broken unit test

[edoardopirovano]

Looks like #1265 broke a unit test in main: https://github.com/github/codeql-action/actions/runs/3113605707/jobs/5048511126

The problem here is that the stub config used in those tests doesn't define the trapCaches field, so we fail to do Object.values() on it. Before the refactoring in the above PR, we were accidentally catching this error in the block that was intended to catch file system errors, but the refactoring changed that behaviour because that catch was moved a bit further in.

Let's do two things:

1. Actually set trapCaches in the stub config.
2. If for some reason it isn't set, let's still be tolerant to that since there's an obvious value of the size of the trap caches when we have none (0). I don't think this can happen in real runs, but let's be cautious (especially since the old behaviour was to be tolerant to this, if only by accident). Dropped this part per discussion below.

We should probably also investigate why the unit tests passed in the above PR which let it get merged. I suspect there's something which should be awaited that isn't, so we fail non-deterministically depending on whether we hit the error before the test finishes.

Merge / deployment checklist

• Confirm this change is backwards compatible with existing workflows.
• Confirm the readme has been updated if necessary.
• Confirm the changelog has been updated if necessary.

[henrymercer]

Since TRAP caching behaves differently on main, have we ensured that when we're testing the Action TRAP caching performs consistently? This might be why we saw the tests fail on main.

[aeisenberg]

If trapCaches is potentially undefined, you should change the type on line 216.

I guess you aren't expecting it to be undefined, but based on how the tests are running, it was. I stil think this should be reflected in the type.

[edoardopirovano]

I think we want the type system to try as much as possible to enforce that trapCaches isn't undefined because it should never actually be undefined, so I'd be inclined to leave the types as they are (whilst still keeping this line for an extra little bit of run time safety). The unit test managed to circumvent the compiler's static checks by doing as unknown as configUtils.Config, but I don't think we want to actually allow it to be undefined without such trickery.

[aeisenberg]

Suggested change

	trapCaches: Partial<Record<Language, string>>,
	trapCaches?: Partial<Record<Language, string>>,

Minor: Not sure if you need Partial here. This might be fine, too:

Suggested change

	trapCaches: Partial<Record<Language, string>>,
	trapCaches?: Record<Language, string>,

[edoardopirovano]

As above, I think the type should record that it is not okay for this to be undefined, so that normal production code that doesn't try and trick the compiler can't set this as undefined.

[edoardopirovano]

Since TRAP caching behaves differently on main, have we ensured that when we're testing the Action TRAP caching performs consistently? This might be why we saw the tests fail on main.

I'm not sure I follow this. The test that broke here isn't actually doing anything with TRAP caching, but it hits the telemetry code path which gets the size of the caches (and since it hasn't put any caches in its stub config, it fails). The actual tests for TRAP caching are unit tests that check the different parts of the behaviour. There unfortunately isn't an end-to-end test as the network calls and interactions with the GitHub Actions cache make it a very hard test to write.

[henrymercer]

I'm not sure I follow this. The test that broke here isn't actually doing anything with TRAP caching, but it hits the telemetry code path which gets the size of the caches (and since it hasn't put any caches in its stub config, it fails). The actual tests for TRAP caching are unit tests that check the different parts of the behaviour. There unfortunately isn't an end-to-end test as the network calls and interactions with the GitHub Actions cache make it a very hard test to write.

I was curious whether we were hitting a code path that depended on actionsUtil.isAnalyzingDefaultBranch(), and therefore only seeing the test failure on main because of that. I was thinking that we'll only upload caches from the default branch, therefore we'll only hit that telemetry code path if we're on the default branch.

[edoardopirovano]

I was curious whether we were hitting a code path that depended on actionsUtil.isAnalyzingDefaultBranch(), and therefore only seeing the test failure on main because of that. I was thinking that we'll only upload caches from the default branch, therefore we'll only hit that telemetry code path if we're on the default branch.

Aha, I see! Yes, you're absolutely right, that's why the test worked on the PR but not on main. If we're not on the default branch we don't bother working out trap_cache_upload_size_bytes because we know we didn't upload anything, so we never call getTotalCacheSize.

The unit tests for TRAP caching do stub out isAnalyzingDefaultBranch (e.g. here) to get us to hit the case we want to test irrespective of what branch we're on, so they're not behaving differently depending on the branch.

[henrymercer]

Thinking about how to fix this, our unit tests shouldn't depend on the default branch. So we should probably update the tests that do things depending on which SHA we're on to mock the SHA that we're on.

That said, I don't know how much work that would be, so I'm happy to merge just the change to set trapCaches to {} for now.

[edoardopirovano]

we should probably update the tests that do things depending on which SHA we're on to mock the SHA that we're on.

I agree. However, taking a look at all our tests and figuring out which ones hit paths that depend on the SHA being worked on and then adding mocking to them is probably a more involved task. I would rather merge this simple fix now to unbreak things and leave that for later work.