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

Improve handling if LONG_TESTS not set #591

Merged
merged 2 commits into from
Apr 24, 2020
Merged

Conversation

terriko
Copy link
Contributor

@terriko terriko commented Apr 23, 2020

Fixes #590

@codecov-io
Copy link

Codecov Report

Merging #591 into master will decrease coverage by 1.52%.
The diff coverage is 96.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #591      +/-   ##
==========================================
- Coverage   86.03%   84.51%   -1.53%     
==========================================
  Files          62       63       +1     
  Lines        2421     2447      +26     
  Branches      488      492       +4     
==========================================
- Hits         2083     2068      -15     
- Misses        218      254      +36     
- Partials      120      125       +5     
Flag Coverage Δ
#longtests 84.51% <96.87%> (+0.04%) ⬆️
#ubuntutests ?
#wintests ?
Impacted Files Coverage Δ
cve_bin_tool/checkers/__init__.py 100.00% <ø> (ø)
test/utils.py 96.66% <83.33%> (-3.34%) ⬇️
cve_bin_tool/checkers/openswan.py 100.00% <100.00%> (ø)
test/test_cli.py 100.00% <100.00%> (ø)
test/test_json.py 90.90% <100.00%> (+0.28%) ⬆️
test/test_scanner.py 93.15% <100.00%> (-2.74%) ⬇️
cve_bin_tool/strings.py 30.00% <0.00%> (-35.00%) ⬇️
cve_bin_tool/checkers/binutils.py 84.21% <0.00%> (-10.53%) ⬇️
cve_bin_tool/extractor.py 66.36% <0.00%> (-8.19%) ⬇️
test/test_file.py 88.00% <0.00%> (-8.00%) ⬇️
... and 8 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 4652d82...58f0b74. Read the comment docs.

@terriko terriko requested a review from pdxjohnny April 23, 2020 22:07
@terriko
Copy link
Contributor Author

terriko commented Apr 23, 2020

Pinging @pdxjohnny for review.

Also, while this works, I'm open to suggestions of ways to make it more elegant. I was trying to make sure you could still search for "LONG_TESTS" and find the right thing for easier debugging, but it does feel a little weird to have it actually secretly be a function now.

@@ -175,13 +175,15 @@ In #99, I've added a _file_test function (to match the existing _binary_test) th
.....
.....
.....
@unittest.skipUnless(int(os.getenv("LONG_TESTS")) > 0, "Skipping long tests")
@unittest.skipUnless(LONG_TESTS() > 0, "Skipping long tests")
Copy link
Member

Choose a reason for hiding this comment

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

We could do something like this too (get from os.environ and provide default of 0 if it doesn't exist):

Suggested change
@unittest.skipUnless(LONG_TESTS() > 0, "Skipping long tests")
@unittest.skipUnless(int(os.environ.get("LONG_TESTS", 0)) > 0, "Skipping long tests")

But I do like the idea of having it be a function, since then we only have to change the logic in one place if we decide to incorporate other checks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the problem was that int(os.environ.get()) fails if LONG_TESTS is not set (because int() won't work on an empty string). We could fit an if in there to handle that, but it looked pretty unwieldly so I went with the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah wait, no I see you're talking a different function than the one that was failing. maybe environ gives something more tractable than getenv did. didn't test that one.

@terriko terriko merged commit b794d43 into intel:master Apr 24, 2020
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.

Tests fail to run if LONG_TESTS is not set
3 participants