Complete test coverage for update-bcd script
#2317
Conversation
Prior to this commit, the expected error in a test for the `inferSupportStatements` function was satisfied by a similar error from the `getSupportMatrix` function. Modify the test to demonstrate the intended error. Add a second test to preserve coverage for `getSupportMatrix`. In both cases, define the expected error more precisely in order to reduce risk for similar false positives in the future.
|
I just pushed up a commit to also verify the return value of the |
|
One of these tests was inaccurate, but it turns out that the corrected version exercises a bug and subsequently fails. Since this patch is intended to improve test coverage only, and since modifying the application logic will obscure that intent, I've opened gh-2326 to correct the bug. I'll be happy to rebase this patch once we merge that fix. |
|
@jugglinmike I've merged #2326 now, if you want to revisit this. |
|
@foolip Thanks for the review! I've resolved the conflicts with a merge commit. While this allowed me to avoid force-pushing and disrupting the review process, it also gummed up the commit history. The easiest resolution is to squash this all together while merging, but that might not be desirable. Part of this work re-arranges tests. That kind of change is prone to silent coverage regression, so some maintainers would prefer to track it with a targeted commit. If you'd prefer to structure the patch according to the commits I originally submitted, let me know, and I'll be happy to rebase and force-push. |
unittest/unit/update-bcd.ts
Outdated
| assert.deepEqual(finalBcd, initialBcd); | ||
| }); | ||
|
|
||
| it('skips complex support scenarios', () => { |
There was a problem hiding this comment.
I think this test will hit this case:
mdn-bcd-collector/update-bcd.ts
Lines 388 to 391 in 19a6230
So the description is correct, but this test would keep passing even if we fix the updater script to handle this scenario, because report doesn't contradict the initialBcd, it just confirms data AbortController isn't in Firefox 92.
Maybe rename this test to 'no update given partial confirmation of removed feature' or something like that, and then make a copy of it that instead tests what happens when initialBcd says that the feature was added in Firefox 100. That should do something different if we fix the updater. (There are lots of combinations, but let's not test all of them.)
There was a problem hiding this comment.
This is a bit tricky: I think the situation you're describing is inaccurate, but your concern is valid and does indeed warrant a new test--just not the test you've suggested.
The test titled "skips removed features" exercises the branch you've highlighted. That test ought to fail when the corresponding TODO comment is addressed.
On the other hand, the test named "skips complex support scenarios" hits the branch immediately before the one you've highlighted:
mdn-bcd-collector/update-bcd.ts
Lines 381 to 384 in 19a6230
(The "TODO" comment there inspired the name of this test.) You're right that this will continue to pass after the "TODO" comment is addressed, so I agree that I should add another test which would fail when the skipping is replaced with more advanced logic. That test needs two "default statements" (to use the terminology of the source code), so I've selected a "support" value of: [{version_added: '93'}, {version_removed: '94'}]. When the "update" script is extended to handle this situation, it ought to change version_added, and the new test ought to fail.
unittest/unit/update-bcd.ts
Outdated
| AbortController: { | ||
| __compat: { | ||
| support: { | ||
| firefox: {version_removed: '91'} |
There was a problem hiding this comment.
version_added is required in BCD. I don't know what will happen exactly, but I don't think we need to test it. And if you add version_added here it will be equivalent to the above test.
But maybe tweak this into the suggested test, where we should end up with two statements representing added-removed-added, but that will actually do nothing for now. Maybe name it 'fails to update removed features' or something.
There was a problem hiding this comment.
Since I couldn't apply your suggestion to the above test, I can take that more direct remediation here.
|
Great tests, thanks @jugglinmike! I've asked to tweak some test names and to clone some tests to reflect my opinion about how the collector should change in the future. Feel free to only make the minimal edits to test names (no new tests) if you feel like it, up to you. |
|
@queengooborg FYI, these tests (and my review comments) show why we can't really rely on |
As we prepare to fix issues with the
update-bcdscript's "mirror" feature (see gh-2280), I felt it was important to have full test coverage for the script.