Test fails if any VENDOR_PRODUCT pair in checker is not lowercase #1032
Conversation
The test fails when the `VENDOR_PRODUCT` pair provided is not lowercase. ( intel#987 )
Codecov Report
@@ Coverage Diff @@
## master #1032 +/- ##
==========================================
- Coverage 87.25% 86.47% -0.79%
==========================================
Files 169 169
Lines 2895 2942 +47
Branches 321 331 +10
==========================================
+ Hits 2526 2544 +18
- Misses 290 316 +26
- Partials 79 82 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
cve_bin_tool/checkers/__init__.py
Outdated
| for items in cls.VENDOR_PRODUCT: | ||
| for vp in items: | ||
| if vp.islower() != True: | ||
| raise AssertionError( |
There was a problem hiding this comment.
Mostly we log errors but don't fail in cve-bin-tool, but the use case for this is "developer added new checker that has a flaw" so we probably do want to halt with an error. So I think raising an error is correct, but assertions in python are a bit of a problem (they get optimized out of production code, so they're used sparingly in test code only) so it probably shouldn't be AssertionError that's raised.
I think in other places we've made our own errors, so probably you want to make something like InvalidCheckerError and raise that instead. Take a look at https://github.com/intel/cve-bin-tool/blob/master/cve_bin_tool/error_handler.py to see what the others look like, and probably add it there.
cve_bin_tool/checkers/__init__.py
Outdated
| for vp in items: | ||
| if vp.islower() != True: | ||
| raise AssertionError( | ||
| "Checker %s has a VENDOR_PRODUCT string that is not lowercase" |
There was a problem hiding this comment.
Please use fstrings here. (In case you missed it, here's where we ask for fstrings in the style guide: https://cve-bin-tool.readthedocs.io/en/latest/CONTRIBUTORS.html#style-guide-for-cve-bin-tool )
|
Hey, @terriko I have done all changes as you suggested. Please review it. |
cve_bin_tool/error_handler.py
Outdated
| @@ -140,6 +144,7 @@ def __exit__(self, exc_type, exc_val, exc_tb): | |||
| SystemExit: -2, | |||
| FileNotFoundError: -3, | |||
| InvalidCsvError: -4, | |||
| InvalidCheckerError: -4, | |||
There was a problem hiding this comment.
I think we probably want InvalidCheckerError to have its own number, so I guess -16 ? InvalidCsvError and InvalidJsonError have the same number presumably because they're very similar parsing errors, but there's no particular reason not to give every error a separate number.
cve_bin_tool/checkers/__init__.py
Outdated
| @@ -2,6 +2,7 @@ | |||
| import collections | |||
| import re | |||
|
|
|||
| from ..error_handler import InvalidCheckerError | |||
There was a problem hiding this comment.
We're trying to move away from using relative imports, so this should be cve_bin_tool.error_handler (and it would be great if you could fix the one below as well, which should be cve_bin_tool.util instead of ..util)
|
And sure, if you spot relative imports go ahead and fix them. There's probably a few left. |
The test fails when the
VENDOR_PRODUCTpairs provided is not lowercase. ( #987 )