Skip to content

Conversation

@metabiswadeep
Copy link
Contributor

Fixes #2700

Add IGNORE_PATTERNS to check against false version patterns.

@metabiswadeep
Copy link
Contributor Author

@terriko Is this okay?

@codecov-commenter
Copy link

codecov-commenter commented Mar 9, 2023

Codecov Report

Merging #2777 (54e514c) into main (d56b9a9) will increase coverage by 0.39%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##             main    #2777      +/-   ##
==========================================
+ Coverage   82.23%   82.62%   +0.39%     
==========================================
  Files         671      672       +1     
  Lines       10597    10652      +55     
  Branches     1426     1438      +12     
==========================================
+ Hits         8714     8801      +87     
+ Misses       1505     1480      -25     
+ Partials      378      371       -7     
Flag Coverage Δ
longtests 82.15% <92.30%> (+6.20%) ⬆️
win-longtests 80.19% <90.90%> (+0.23%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cve_bin_tool/checkers/__init__.py 91.48% <60.00%> (-3.86%) ⬇️
cve_bin_tool/util.py 79.48% <100.00%> (+0.53%) ⬆️
test/test_checkers.py 100.00% <100.00%> (ø)

... and 12 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@terriko terriko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking fantastic! I think it needs a bit more of a test. I think maybe a fake checker that runs against something like this:

myProduct 1.2.3
Ignore This Line myProduct 5.6

And we want it to return only myProduct 1.2.3 and ignore myProduct 5.6 to show that the ignore part is dropping just the expected line.

But it's the end of my work day and I'm a bit fried so I'm not sure I'm explaining this well. Feel free to ping me if that didn't make any sense to you.

@metabiswadeep
Copy link
Contributor Author

This is looking fantastic! I think it needs a bit more of a test. I think maybe a fake checker that runs against something like this:

myProduct 1.2.3
Ignore This Line myProduct 5.6

And we want it to return only myProduct 1.2.3 and ignore myProduct 5.6 to show that the ignore part is dropping just the expected line.

But it's the end of my work day and I'm a bit fried so I'm not sure I'm explaining this well. Feel free to ping me if that didn't make any sense to you.

@terriko The fake checker that is needed, how do I exactly make it? Do I need to create a separate checker file for it along with seperate file with mapping parameters in test_data?

@terriko
Copy link
Collaborator

terriko commented Mar 23, 2023

Hm, that's a good question. I was thinking that you'd just make the checker in the test file and then load it directly, but I'm not actually sure if that's doable...

@terriko
Copy link
Collaborator

terriko commented Mar 23, 2023

Okay, so looking at the code a bit to give you a better answer about this:

In the test file, you should be able to make a new checker as a subclass of Checker (same as what we do in the individual checker files, but you'd be making a local one just for the test). Then instead of using the tool to call all the checkers, you should be able to have the test call that specific checker's functions (e.g. myFakeChecker.get_version) and make sure it does the right thing.

You'd give it something like the lines I listed above, with the expected result listed above.

Copy link
Collaborator

@terriko terriko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some

 =========================== short test summary info ============================
ERROR test/test_checkers.py - TypeError: 'type' object is not subscriptable
ERROR test/test_checkers.py - TypeError: 'type' object is not subscriptable
ERROR test/test_checkers.py - TypeError: 'type' object is not subscriptable
ERROR test/test_checkers.py - TypeError: 'type' object is not subscriptable
===== 1558 passed, 77 skipped, 33 warnings, 4 errors in 977.26s (0:16:17) ======

Not sure at a glance what's going on here, but I'm guessing from the fact that the fail happened in 3.7 that it might be that test_checkers.py is missing this line to make it understand the more modern type syntax:

from __future__ import annotations

@metabiswadeep
Copy link
Contributor Author

@terriko Is it okay now?

@metabiswadeep
Copy link
Contributor Author

@terriko The tests are passing now.

@metabiswadeep
Copy link
Contributor Author

@terriko Is there any other modification necessary for this?

@metabiswadeep metabiswadeep requested a review from terriko April 26, 2023 16:31
Copy link
Collaborator

@terriko terriko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I'm going to make some tweaks to the docs part to indicate that IGNORE_PATTERNS is optional and explain one common case when we expect people to use it, but I should be able to merge those and go. thanks for working on this!

Comment on lines +106 to +108
for i in ignore:
if str(i) in str(new_guess) or str(new_guess) in str(i):
new_guess = ""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if we should remove the ignore patterns first and avoid the new_guess="" syntax, but upon thinking about it this seems computationally better since it means we only have to check them against a few lines. Good thinking!

@terriko
Copy link
Collaborator

terriko commented May 10, 2023

Phew, this one got stuck in waiting for CI for quite a while. merging now; thank you again!

@terriko terriko merged commit 6cfdc43 into ossf:main May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: checker version "do not match" patterns?

3 participants