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

Tests: first pass at improving coverage #47

Merged
merged 12 commits into from Jan 15, 2020
Merged

Tests: first pass at improving coverage #47

merged 12 commits into from Jan 15, 2020

Conversation

teknico
Copy link
Collaborator

@teknico teknico commented Dec 30, 2019

This is a first pass at improving test coverage. Part of #38.

I added a .coveragerc config file which, among other things, excludes the test files themselves from coverage. This brought the overall coverage down from a theoretical 74% to a more realistic 70%.

It also excludes lines raising several errors: AssertionError, NotImplementedError, CrazyKeyError and ParameterRangeError. I then changed a number of exceptions to AssertionError and NotImplementedError, let me know if this approach makes sense.

I manually excluded some minor methods from coverage with the nocover comment.

I also added and extended several tests, which also increased coverage. It currently stands at 71%.

Here is the command I'm using (it needs the pytest-cov plugin), run from the directory above the repo:

$ pytest --cov-config=./tinydecred/.coveragerc --cov-report=html --cov=tinydecred ./tinydecred/tests/

UPDATE 1: more Exceptions converted into AssertionErrors, added more tests. Coverage is now at 72%.

UPDATE 2: split test_pydecred.py into test_calc.py, text_txscript.py and test_vsp.py. text_txscript.py is quite large already, no reason to group unrelated tests with it.

UPDATE 3: replaced all Unimplemented errors in wallet/api.py with the built-in NotImplementedError, and removed the Unimplemented class itself. Coverage is now at 73%.

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

Some really nice improvements here.

crypto/crypto.py Outdated Show resolved Hide resolved
crypto/crypto.py Outdated Show resolved Hide resolved
pydecred/txscript.py Show resolved Hide resolved
pydecred/txscript.py Outdated Show resolved Hide resolved
pydecred/txscript.py Outdated Show resolved Hide resolved
pydecred/wire/msgtx.py Outdated Show resolved Hide resolved
tests/unit/crypto/test_bytearray.py Outdated Show resolved Hide resolved
@teknico teknico marked this pull request as ready for review January 2, 2020 11:44
@teknico
Copy link
Collaborator Author

teknico commented Jan 2, 2020

Thanks. I implemented all the changes requested in the pre-review, coverage is back to 72%.

Although there's more work to do, this PR is fairly large and ready for actual review and merge. It's also OK for it to wait until the other large PRs in the pipeline are in.

@buck54321
Copy link
Member

It's also OK for it to wait until the other large PRs in the pipeline are in

I think I'm going to take you up on this again. At least for #30, but maybe for #42 as well.

@buck54321 buck54321 mentioned this pull request Jan 9, 2020
crypto/bytearray.py Outdated Show resolved Hide resolved
crypto/crypto.py Outdated Show resolved Hide resolved
crypto/secp256k1/field.py Show resolved Hide resolved
pydecred/txscript.py Outdated Show resolved Hide resolved
pydecred/wire/msgblock.py Outdated Show resolved Hide resolved
pydecred/wire/msgtx.py Outdated Show resolved Hide resolved
pydecred/wire/wire.py Outdated Show resolved Hide resolved
tests/unit/pydecred/wire/test_wire.py Outdated Show resolved Hide resolved
@buck54321
Copy link
Member

#42 is merged. Sorry for the messy rebase.

@teknico
Copy link
Collaborator Author

teknico commented Jan 14, 2020

Rebased again, fixed all conflicts, ready for re-review.

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

Pretty much ready. Loving the new coverage too, and you've already found a bug.

.coveragerc Show resolved Hide resolved
pydecred/txscript.py Outdated Show resolved Hide resolved
pydecred/wire/msgtx.py Outdated Show resolved Hide resolved
tests/unit/pydecred/wire/test_msgtx.py Show resolved Hide resolved
tests/unit/pydecred/wire/test_wire.py Show resolved Hide resolved
tests/unit/util/test_encode.py Show resolved Hide resolved
@buck54321 buck54321 merged commit 385bcc1 into decred:master Jan 15, 2020
@teknico teknico deleted the improve-test-coverage branch January 15, 2020 12:44
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

4 participants