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

Quiet Mode should not generate any output #830

Merged
merged 2 commits into from Jul 29, 2020

Conversation

Niraj-Kamdar
Copy link
Contributor

@Niraj-Kamdar Niraj-Kamdar commented Jul 18, 2020

Depends on #851

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2020

Codecov Report

Merging #830 into master will increase coverage by 5.21%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #830      +/-   ##
==========================================
+ Coverage   83.37%   88.58%   +5.21%     
==========================================
  Files         156      156              
  Lines        2574     2585      +11     
  Branches      277      280       +3     
==========================================
+ Hits         2146     2290     +144     
+ Misses        360      231     -129     
+ Partials       68       64       -4     
Flag Coverage Δ
#longtests 88.58% <100.00%> (+5.21%) ⬆️

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

Impacted Files Coverage Δ
test/test_data/openssl.py 100.00% <ø> (ø)
cve_bin_tool/cli.py 84.48% <100.00%> (+5.17%) ⬆️
cve_bin_tool/cve_scanner.py 81.35% <100.00%> (+0.48%) ⬆️
cve_bin_tool/cvedb.py 86.25% <100.00%> (+32.40%) ⬆️
cve_bin_tool/egg_updater.py 100.00% <100.00%> (ø)
test/test_cli.py 100.00% <100.00%> (+17.07%) ⬆️
test/utils.py 97.05% <100.00%> (ø)
cve_bin_tool/extractor.py 60.62% <0.00%> (+1.57%) ⬆️
cve_bin_tool/version_scanner.py 86.08% <0.00%> (+1.73%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45cf861...fa769a2. Read the comment docs.

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.

As well as the comments, could you add a quiet mode test (or tests per section) to show that these work, since I'm guessing we weren't triggering a fail correctly before?

[Panel(f"[yellow]{cve.cve_number}[/yellow]") for cve in cves]
),
)
if self.error_mode.value > 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

The code looks good, but I'd like some comments in here to mention quiet mode to explain why these are like this, mostly so it's there as a reminder for folk adding any further output.

):
total_tasks = len(tasks)

if self.error_mode.value > 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

Also mention quiet mode here.

@@ -310,7 +316,12 @@ def populate_db(self):
"""
del_cve_range = "DELETE from cve_range where CVE_number=?"

for year in track(self.nvd_years(), description="Updating CVEs from NVD..."):
if self.error_mode.value > 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

@Niraj-Kamdar
Copy link
Contributor Author

@terriko, here problem is we are asserting logger shouldn't produce any output in quiet mode but we are supposed to assert there should be no output produced on console in quiet mode. This isn't very hard to do if we just use unittest or pytest but we are using pytest to run unittest which is making things more complicated. I recommend switching to pytest in this case because it provides easy way to capture stdout and stderr.

@terriko
Copy link
Contributor

terriko commented Jul 23, 2020

I think we talked about this yesterday, but just to confirm: I agree that switching to pytest and updating the quiet mode test sounds like the correct solution.

Comment on lines +244 to +250

# Make sure log is empty
assert not caplog.records

# Make sure nothing is getting printed on stdout or stderr
captured = capsys.readouterr()
assert not (captured.out or captured.err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated tests for quiet mode.

@Niraj-Kamdar
Copy link
Contributor Author

Niraj-Kamdar commented Jul 24, 2020

CI is failing due to following error:

>       assert not (captured.out or captured.err)
E       assert not ('' or "warning: no files found matching 'test/binaries/*.c'\n")
E        +  where '' = CaptureResult(out='', err="warning: no files found matching 'test/binaries/*.c'\n").out
E        +  and   "warning: no files found matching 'test/binaries/*.c'\n" = CaptureResult(out='', err="warning: no files found matching 'test/binaries/*.c'\n").err

I am not sure from where this warning is being generated. I am not facing any errors locally.

@Niraj-Kamdar
Copy link
Contributor Author

Niraj-Kamdar commented Jul 24, 2020

I found out the bug. In egg_updater I have just redirected stdout and I forget to redirect stderr which was producing warning sometimes. At least we can be sure that new quiet mode test is working correctly.

@terriko terriko merged commit 1eeace8 into intel:master Jul 29, 2020
@Niraj-Kamdar Niraj-Kamdar deleted the cve_scanner branch July 29, 2020 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants