fix: fixed quiet_mode test to ignore unimportant logs

[inosmeet]

Fixes: #2382

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (91cabcb) 80.37% compared to head (436c005) 80.20%.
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3795      +/-   ##
==========================================
- Coverage   80.37%   80.20%   -0.18%     
==========================================
  Files         808      808              
  Lines       11977    11983       +6     
  Branches     1595     1598       +3     
==========================================
- Hits         9627     9611      -16     
- Misses       1923     1955      +32     
+ Partials      427      417      -10     

Flag	Coverage Δ
longtests	80.20% <100.00%> (+0.36%)	⬆️
win-longtests	?

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[terriko]

This test doesn't really test what we need here.

If you set stdout/stderr to /dev/null then sure, you get the same effect that the -q flag is supposed to have. But it doesn't test whether -q itself is working because it's dumping all the output into the void.

It might help to imagine that someone has quiet mode hooked up to an email bot: you want it to email you if there's some kind of catastrophic failure of cve-bin-tool that we couldn't predict. But you don't want it to mail you every day with boring "downloading" messages or "unpacked a file" messages or "connection failed, retrying." For any error we can predict (like periodic network timeouts), you only want an appropriate error code to tell you if something went wrong. (e..g imagine the error codes being shown on a dashboard of thousands of runs.)

In our case, the quiet mode test is supposed to make sure we've ensured all output goes with an appropriate log priority (e.g. no rogue print statements), and that all exceptions are handled correctly (e.g. no surprise python stack traces; we should print a log.error and if the issue halts the tool entirely we should set an appropriate error code)

[inosmeet]

There was some misunderstanding:

• While reproducing this one, quiet mode was running as expected, however the tests were logging some non critical logs.
• when trying cve-bin-tool -q -u now without network connection, didn't print any logs(only tracebacks).

the problem is with our implementation of quiet_mode:

• we are only restricting logs of our LOGGER to CRITICAL level and the logs we are getting is from another logging implementation.

So, I'm adding a commit to address that.

[inosmeet]

all exceptions are handled correctly (e.g. no surprise python stack traces; we should print a log.error and if the issue halts the tool entirely we should set an appropriate error code)

Regarding stack traces, there is inconsistency in the code, many parts are lacking effective error handling and corresponding issues can be made.

[terriko]

all exceptions are handled correctly (e.g. no surprise python stack traces; we should print a log.error and if the issue halts the tool entirely we should set an appropriate error code)

Regarding stack traces, there is inconsistency in the code, many parts are lacking effective error handling and corresponding issues can be made.

I agree that there's a lot of room for improvement but I should probably be clear about my priorities:

• If an error is something that won't stop a scan BUT might affect the results, we need to log it appropriately for the user and potentially include notes in the final report. (e.g. if a file couldn't be scanned)
• If an error is something that shouldn't stop a scan and doesn't really affect the results (e.g. a timeout that we then retried and it was fine) then we can log at a lower level (warn or debug) and do not need to include it in the reports.
• If an error is catastrophic and stops the scan, then a stack trace is ok. It's often not the best option (for example, feat: add message about mirror when nvd is down #3547) but if you can't make an error message that's better than the stack trace, I'd rather leave the stack trace in place to help with debugging. (e..g. I don't want to just add a giant catch-all around cli.py that pretty prints errors, because that would hinder our ability to debug anything new and unexpected)

In that last case, violating quiet mode would be appropriate regardless.

[terriko]

There was some misunderstanding:

* While reproducing this one, quiet mode was running as expected, however the tests were logging some _non critical_ logs.

* when trying `cve-bin-tool -q -u now` without network connection, didn't print any logs(only tracebacks).

the problem is with our implementation of quiet_mode:

* we are only restricting logs of our `LOGGER` to `CRITICAL` level and the logs we are getting is from another logging implementation.

So, I'm adding a commit to address that.

Yeah, commits to address the extra log messages is the correct way to go here.

Setting stderr/stout to /dev/null would mean that even if quiet mode was broken then this test would pass, and that would make it pretty useless as a regression test.

[inosmeet]

If an error is catastrophic and stops the scan, then a stack trace is ok. It's often not the best option (for example, #3547) but if you can't make an error message that's better than the stack trace, I'd rather leave the stack trace in place to help with debugging. (e..g. I don't want to just add a giant catch-all around cli.py that pretty prints errors, because that would hinder our ability to debug anything new and unexpected)

yes, by using critical logs, we can reduce the noise without affecting debugging quality.

I think that these should be separate as issue for network related problems is already there #1431
or are you considering to include that in this PR?

[inosmeet]

Also, do we want anything else in this one?

[terriko]

Yes, I think this will work better. Thanks!