Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: basic execution test for EPSS #4484 #4510

Merged
merged 16 commits into from
Dec 18, 2024

Conversation

weichslgartner
Copy link
Contributor

Adds test described in #4484.
Add a test to the cli tests to check the EPSS functionality: It first tests if the update of EPSS source runs without errors (regression test for #4473).
Then checks for an example SBOM if EPSS values are written to csv report.
I tried to disable the other data sources, but needed NVD. Any suggestions to minimize the test are welcome.

Should fail on current main, should pass after #4475 is merged.
Tested it locally with https://github.com/weichslgartner/cve-bin-tool/tree/basic_execution_test_for_EPSS

Add a test to the cli tests to check the EPSS functionality:
It first tests if the update of EPSS source runs without errors
(regression test for intel#4473).
Then checks for an example SBOM if EPSS values are written to csv report.
@weichslgartner weichslgartner force-pushed the basic_execution_test_for_EPSS branch from 31c09d7 to e35ae71 Compare October 14, 2024 21:23
test/test_cli.py Outdated Show resolved Hide resolved
Copy link
Contributor

@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 the test is failing:

 =========================== short test summary info ============================
FAILED test/test_cli.py::TestCLI::test_basic_epss - ValueError: could not convert string to float: ''
============= 1 failed, 22 passed, 11 skipped in 68.76s (0:01:08) ==============

@weichslgartner
Copy link
Contributor Author

@terriko I branched from main when the other fix was not yet merged so this was expected. Yet it didn't fail during the update (as was skipped because of the cache), but only in the step when it checked if the EPSS value from the csv file. I will update with the main branch, then this test should pass.

@weichslgartner
Copy link
Contributor Author

@terriko maybe

assert (
could be run with cap level error?

@terriko
Copy link
Contributor

terriko commented Oct 15, 2024

Ah, my bad -- for some reason I had it in my head that this had been updated but of course it hadn't. (that's what I get for multitasking too much this morning while scanning PRs.) Letting things re-run now so I can see what's up.

Copy link
Contributor

@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.

Still getting the same error:

=========================== short test summary info ============================
FAILED test/test_cli.py::TestCLI::test_basic_epss - ValueError: could not convert string to float: ''
============= 1 failed, 22 passed, 11 skipped in 68.77s (0:01:08) ==============

I haven't dug into it, but I do see that error often when we forget to trim whitespace on a string, so there may be a wayward " " somewhere? Or as you say, maybe the log level isn't giving you the correct messages and you'll need to swap it?

@weichslgartner
Copy link
Contributor Author

@terriko It seems it didn't update the EPSS data because the cache seems to be still old (before the EPSS fix). The Value error comes from parsing values out of the generated csv files. The two last values should be epss_percentile and epss_probability (it also asserts the header and they didn't trigger). If EPSS failed there are no values there hence the conversion fails. (I will create a better error message here). I reproduced the this fail locally with an old cache with not epss data. Only if I do an "-u now" update it downloads the data correctly. So having a way to force epss update without have to download all the cve data would be good.

@terriko
Copy link
Contributor

terriko commented Oct 18, 2024

Hm, the cache was broken but I fixed it earlier this week and I did update last night. Let me go pull latest and then try re-running this.

@terriko
Copy link
Contributor

terriko commented Oct 18, 2024

(I'm wondering if there's something else up with caching that I'm not seeing, though, because I'm still having some weirdness on jobs.)

@terriko
Copy link
Contributor

terriko commented Oct 18, 2024

Okay, cache has updated. I'm going to try re-running the failing tests again and see if that unstuck it or if there's more to do.

@terriko
Copy link
Contributor

terriko commented Oct 28, 2024

Kicking off the tests again.

@weichslgartner
Copy link
Contributor Author

@terriko
I did a little debugging under windows and seems there the csv file contains empty rows cause by (\n\n), first I thought is is a windows line ending \r\n issue, but splitlines should take care of the issue.
The file looks like:

vendor,product,version,location,cve_number,severity,score,source,cvss_version,cvss_vector,paths,remarks,comments,epss_probability,epss_percentile

gnu,glibc,2.11.1,NotFound,CVE-2009-5029,MEDIUM,6.8,NVD,2,AV:N/AC:M/Au:N/C:P/I:P/A:P,,NewFound,,0.00801,0.82134

gnu,glibc,2.11.1,NotFound,CVE-2009-5155,HIGH,7.5,NVD,3,CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H,,NewFound,,0.00469,0.76073

gnu,glibc,2.11.1,NotFound,CVE-2010-0296,HIGH,7.2,NVD,2,AV:L/AC:L/Au:N/C:C/I:C/A:C,,NewFound,,0.00044,0.11027

I added now a step to filter out empty lines and now the windows tests run at least locally on my windows machine.
Can check why there are empty lines in the csv file under windows tomorrow.

@weichslgartner
Copy link
Contributor Author

seems to be this issue: https://stackoverflow.com/a/30930022

@weichslgartner
Copy link
Contributor Author

I opened #4557 for the csv issue under windows

@terriko
Copy link
Contributor

terriko commented Nov 19, 2024

kicking off the tests again. Not sure if I need to rebase the branch to get the other fix in so I'm flagging this for myself so I double-check if it fails.

@terriko terriko added the awaiting maintainer Need a maintainer to respond / help out label Nov 19, 2024
@terriko
Copy link
Contributor

terriko commented Nov 19, 2024

K, so the long tests failed again. Going to rebase the branch and try again.

@terriko terriko dismissed their stale review December 17, 2024 22:25

Dismissing review in case it's what's blocking the tests from re-running.

@terriko
Copy link
Contributor

terriko commented Dec 18, 2024

One last time! I hope! I'm going to bump the branch for the new split up long tests and see where we're at now that they're actually behaving consistently.

Copy link
Contributor

@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.

Okay, I think we're finally ready to merge. Thank you so much for working on this and yoru extreme patience with the CI system!

@terriko terriko merged commit 8791957 into intel:main Dec 18, 2024
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting maintainer Need a maintainer to respond / help out
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants