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

Add tests for some command line arguments #222

Merged
merged 1 commit into from Feb 18, 2018
Merged

Conversation

@mayeut
Copy link
Contributor

@mayeut mayeut commented Feb 17, 2018

Test version and help options are printing expected values on stdout.
Test invalid arguments return the correct exit code and print the
correct error message on stderr.

@codecov
Copy link

@codecov codecov bot commented Feb 17, 2018

Codecov Report

Merging #222 into master will increase coverage by 2.25%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #222      +/-   ##
==========================================
+ Coverage   81.31%   83.56%   +2.25%     
==========================================
  Files           2        3       +1     
  Lines        1145     1223      +78     
  Branches      247      248       +1     
==========================================
+ Hits          931     1022      +91     
+ Misses        149      140       -9     
+ Partials       65       61       -4
Impacted Files Coverage Δ
gcovr/__main__.py 80.57% <100%> (+1.25%) ⬆️
gcovr/tests/test_args.py 100% <100%> (ø)

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 16a282d...1181fea. Read the comment docs.

Copy link
Member

@latk latk left a comment

These test cases are great! It's wonderful how simple the tests can be since we have a main() function :)

However, I'm not a brilliant Pythonist so I found the test infrastructure a bit confusing, especially the BufferedStringIO class and capture() context manager. Please see the various inline comments below. In the future, a small comment explaining why we need a given function or class would be super helpful.



@contextmanager
def ioscope(file):

This comment has been minimized.

@latk

latk Feb 17, 2018
Member

files already are context managers, so I think this function is a no-op?

This comment has been minimized.

@mayeut

mayeut Feb 18, 2018
Author Contributor

StringIO has no context manager on python 2

try:
e = None
main(args)
except BaseException as exception:

This comment has been minimized.

@latk

latk Feb 17, 2018
Member

It would usually be preferable to only catch Exception subclasses, as this will also catch Ctrl-C (KeyboardInterrupt). We do however want to intercept SystemExit. We later access the SystemExit.code field. So should capture() only handle SystemExit, rather than all exceptions? The tests will still fail if we get an unexpected exception.

This comment has been minimized.

@mayeut

mayeut Feb 18, 2018
Author Contributor

I'll change this.

self.exception = exception


class StringIOBuffered(StringIO):

This comment has been minimized.

@latk

latk Feb 17, 2018
Member

Is this class needed? I think a StringIO.getvalue() call is sufficient in capture(), no need to overrride StringIO.close().

This comment has been minimized.

@mayeut

mayeut Feb 18, 2018
Author Contributor

StringIO.getvalue() will raise an exception if StringIO.close() has been called. StringIOBuffered offers StringIOBuffered.data() which will not raise an exception if close() has been called.

main(args)
except BaseException as exception:
e = exception
yield CaptureObject(sys.stdout.data(), sys.stderr.data(), e)

This comment has been minimized.

@latk

latk Feb 17, 2018
Member

might be simpler as:

yield CaptureObject(newout.getvalue(), newerr.getvalue(), e)
with capture(['--fail-under-branch', 'nan']) as c:
self.assertEqual(c.out, '')
self.assertTrue('not in range [0.0, 100.0]' in c.err)
self.assertNotEqual(c.exception.code, 0)

This comment has been minimized.

@latk

latk Feb 17, 2018
Member

In some places we have assertNotEqual(code, 0) and others assertEqual(code, 1). That tripped me up because I overlooked the "not". Can we use the assertNotEqual(code, 0) everywhere where a specific exit code is not necessary?

This comment has been minimized.

@mayeut

mayeut Feb 18, 2018
Author Contributor

I used assertEqual(code, 1) when gcovr calls sys.exit(1) and assertNotEqual(code, 0) when parsing fails (we do not know what optparse will call sys.exit with)

def capture(args):
newout = StringIOBuffered()
with ioscope(newout):
with ioscope(StringIOBuffered()) as newerr:

This comment has been minimized.

@latk

latk Feb 17, 2018
Member

Can have multiple context managers in one "with" statement, reduces indentation:

with StringIO() as newout, StringIO() as newerr:

This comment has been minimized.

@mayeut

mayeut Feb 18, 2018
Author Contributor

Nice, will update this



@contextmanager
def capture(args):

This comment has been minimized.

@latk

latk Feb 17, 2018
Member

Does this need to be a context manager? It seems to me like the capture data could simply be returned?

This comment has been minimized.

@mayeut

mayeut Feb 18, 2018
Author Contributor

you're right, will update this

with ioscope(newout):
with ioscope(StringIOBuffered()) as newerr:
err, sys.stderr = sys.stderr, newerr
out, sys.stdout = sys.stdout, newout

This comment has been minimized.

@latk

latk Feb 17, 2018
Member

reassigning sys.stdout is yucky, but there's currently no alternative :(
We'd need to introduce a better logging mechanism first, compare #94.

@mayeut mayeut force-pushed the mayeut:test-cli-args branch 4 times, most recently from f6d8d01 to 0ebf308 Feb 18, 2018
Test version and help options are printing expected values on stdout.
Test invalid arguments return the correct exit code and print the
correct error message on stderr.
@mayeut mayeut force-pushed the mayeut:test-cli-args branch from 0ebf308 to 1181fea Feb 18, 2018
@latk
latk approved these changes Feb 18, 2018
Copy link
Member

@latk latk left a comment

Thank you for the extra explanations. This makes a lot of sense now. Great work!

@latk latk merged commit 9673291 into gcovr:master Feb 18, 2018
4 checks passed
4 checks passed
@codecov
codecov/patch 100% of diff hit (target 81.31%)
Details
@codecov
codecov/project 83.56% (+2.25%) compared to 16a282d
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mayeut mayeut deleted the mayeut:test-cli-args branch Feb 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants