Don't compare internal and dto types in variant-analysis-history.test.ts #3293
Conversation
| join(STORAGE_DIR, "workspace-query-history.json"), | ||
| ).queries; | ||
| ).queries.map(mapQueryHistoryVariantAnalysisToDomainModel); |
There was a problem hiding this comment.
One thing I'm not totally clear on is what exactly we're testing. So is it appropriate to use mapQueryHistoryVariantAnalysisToDomainModel to generate the expected data, or are we basically just calling the same function twice and comparing the result?
I also tried writing out the expected data manually and explicitly, instead of loading it from this file. The issues I ran into were:
- we duplicate a lot of data (but that's kinda the point)
- the data contains some
PLACEHOLDERs in the file paths, because we don't know where the file will be written so we can't hard-code the path to the QL files
There was a problem hiding this comment.
To be honest, I don't think using mapQueryHistoryVariantAnalysisToDomainModel is any different from what the test is doing right now, which is also essentially checking some data against itself.
At least we're still verifying that it can parse the data without erroring and at least we get something out at the other end 🤷🏼
robertbrignull
deleted the
robertbrignull/variant-analysis-history-test-dto
branch
In
variant-analysis-history.test.tswe are reading the raw query history DTO types from a file and then trying to compare them to the internal types from the query history manager. Onmainthis happens to work because the types are the same, but the whole point of having these two types is that they don't have to be the same.The "workspace-query-history.json" file is where the query history manager also loads its data from. However in the tests we never actually want it as this raw data and instead just want to make sure it's done the loading correctly.
Similar to #3290, I found this because of a test failure on https://github.com/github/vscode-codeql/actions/runs/7699342606/job/20983096500?pr=3287 where the internal types are changing but the DTO types are not.
Checklist
ready-for-doc-reviewlabel there.