Fix broken unit test - #1266
Fix broken unit test#1266edoardopirovano merged 1 commit into
Conversation
Sorry, something went wrong.
henrymercer
left a comment
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| trapCaches: Partial<Record<Language, string>>, | ||
| logger: Logger | ||
| ): Promise<number> { | ||
| if (!trapCaches) return 0; |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| @@ -216,6 +216,7 @@ export async function getTotalCacheSize( | |||
| trapCaches: Partial<Record<Language, string>>, | |||
There was a problem hiding this comment.
| trapCaches: Partial<Record<Language, string>>, | |
| trapCaches?: Partial<Record<Language, string>>, |
Minor: Not sure if you need Partial here. This might be fine, too:
| trapCaches: Partial<Record<Language, string>>, | |
| trapCaches?: Record<Language, string>, |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
henrymercer
left a comment
There was a problem hiding this comment.
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.
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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:
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