test: add tests #10
test: add tests #10
Conversation
There was a problem hiding this comment.
Okay, I've got a number of recommendations for you, but I don't think any of them are blockers for initial merge so much as they're opportunities for refactoring to make the tests require less maintenance in the future as cve data changes. So I'm going to go ahead and merge this, then open an issue for refactoring in a future PR.
| @@ -85,7 +85,7 @@ | |||
| tests: | |||
|
|
|||
| # (optional) list skipped test IDs here, eg '[B101, B406]': | |||
| # skips: ['B603', 'B607', 'B404', "B608"] | |||
| skips: ['B603', 'B607', 'B404', "B608"] | |||
There was a problem hiding this comment.
Do we actually need to disable B608? I can see that we'd need to do the subprocess ones but B608 is sqlite-related and I don't think anything in this repo talks to the database directly?
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| assert available_fixes == self.AVAILABLE_FIXES |
There was a problem hiding this comment.
This is likely to be a bit brittle as issues change over time, as we've seen with similar tests in the main tool. I'd suggest for future proofing that we change it to something more like...
| assert available_fixes == self.AVAILABLE_FIXES | |
| assert self.AVAILABLE_FIXES.issubset(available_fixes) |
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| assert available_fixes == self.AVAILABLE_FIXES_WITH_SHRINKED_CONSOLE_OUTPUT |
There was a problem hiding this comment.
Same deal as above. Not sure offhand if we'd have to convert available_fixes to be a set explicitly.
| assert available_fixes == self.AVAILABLE_FIXES_WITH_SHRINKED_CONSOLE_OUTPUT | |
| assert self.AVAILABLE_FIXES_WITH_SHRINKED_CONSOLE_OUTPUT.issubset(available_fixes) |
| ) | ||
| sarif_generator.write_file(self.tmp_dir / "output.sarif") | ||
| with open(self.tmp_dir / "output.sarif") as fd: | ||
| assert fd.read() == self.NO_CVE_SARIF_OUTPUT |
There was a problem hiding this comment.
This may have the same brittleness as the other tests (both on this line and the other assert on line 55 below), but we'd have to convert the text to a set of lines before using the same trick as with the test_cve_bin_tool.py file above.
| ] | ||
| ) | ||
| with open(self.tmp_dir / "output.sarif") as fd: | ||
| assert fd.read() == self.SARIF_OUTPUT |
There was a problem hiding this comment.
Potentially the same brittleness concern here as elsewhere. I don't know offhand if there's an equivalent to issubset for json or you'd have to load both into a set structure to do the same set arithmetic.
No description provided.