Add tests for some command line arguments #222
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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.
gcovr/tests/test_args.py
Outdated
|
|
||
|
|
||
| @contextmanager | ||
| def ioscope(file): |
There was a problem hiding this comment.
files already are context managers, so I think this function is a no-op?
There was a problem hiding this comment.
StringIO has no context manager on python 2
gcovr/tests/test_args.py
Outdated
| try: | ||
| e = None | ||
| main(args) | ||
| except BaseException as exception: |
There was a problem hiding this comment.
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.
gcovr/tests/test_args.py
Outdated
| self.exception = exception | ||
|
|
||
|
|
||
| class StringIOBuffered(StringIO): |
There was a problem hiding this comment.
Is this class needed? I think a StringIO.getvalue() call is sufficient in capture(), no need to overrride StringIO.close().
There was a problem hiding this comment.
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.
gcovr/tests/test_args.py
Outdated
| main(args) | ||
| except BaseException as exception: | ||
| e = exception | ||
| yield CaptureObject(sys.stdout.data(), sys.stderr.data(), e) |
There was a problem hiding this comment.
might be simpler as:
yield CaptureObject(newout.getvalue(), newerr.getvalue(), e)
gcovr/tests/test_args.py
Outdated
| 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
gcovr/tests/test_args.py
Outdated
| def capture(args): | ||
| newout = StringIOBuffered() | ||
| with ioscope(newout): | ||
| with ioscope(StringIOBuffered()) as newerr: |
There was a problem hiding this comment.
Can have multiple context managers in one "with" statement, reduces indentation:
with StringIO() as newout, StringIO() as newerr:There was a problem hiding this comment.
Nice, will update this
gcovr/tests/test_args.py
Outdated
|
|
||
|
|
||
| @contextmanager | ||
| def capture(args): |
There was a problem hiding this comment.
Does this need to be a context manager? It seems to me like the capture data could simply be returned?
There was a problem hiding this comment.
you're right, will update this
gcovr/tests/test_args.py
Outdated
| with ioscope(newout): | ||
| with ioscope(StringIOBuffered()) as newerr: | ||
| err, sys.stderr = sys.stderr, newerr | ||
| out, sys.stdout = sys.stdout, newout |
There was a problem hiding this comment.
reassigning sys.stdout is yucky, but there's currently no alternative :(
We'd need to introduce a better logging mechanism first, compare #94.
mayeut
force-pushed
the
test-cli-args
branch
4 times, most recently
from
f6d8d01
to
0ebf308
Compare
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
force-pushed
the
test-cli-args
branch
from
0ebf308
to
1181fea
Compare
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.