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

ENH: stop using import_doi when testing format_bibtex with utf8 #121

Merged
merged 1 commit into from Aug 16, 2017

Conversation

Projects
None yet
4 participants
@mslw
Contributor

mslw commented Aug 16, 2017

Trying to resolve #119

I noticed that even without any changes, the test in question passes for me locally (python 3.6), so the errors may have been due to some temporary issue with querying zenodo.

I copy-pasted the output of format_bibtex() into the function test_format_bibtex_with_utf_characters(), so that this test won't fail when there is a problem with that zenodo query (rather than a problem with handling utf-8 in format_bibtex(), which we are explicitly testing for) - similar to what was done in test_format_bibtex_zenodo_doi().

On the other hand, perhaps testing whether import_doi() is successful should still be done?

ENH: stop using import_doi when testing format_bibtex with utf8
Hardcoded the output of format_bibtex inside the function
test_format_bibtex_with_utf_characters
so that this test won't fail when there is a problem with e.g.
zenodo queries (rather than with utf-8 processing itself).
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 16, 2017

Coverage Status

Changes Unknown when pulling 861620d on mslw:enh_test_utf8 into ** on duecredit:master**.

coveralls commented Aug 16, 2017

Coverage Status

Changes Unknown when pulling 861620d on mslw:enh_test_utf8 into ** on duecredit:master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 16, 2017

Coverage Status

Changes Unknown when pulling 861620d on mslw:enh_test_utf8 into ** on duecredit:master**.

coveralls commented Aug 16, 2017

Coverage Status

Changes Unknown when pulling 861620d on mslw:enh_test_utf8 into ** on duecredit:master**.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 16, 2017

Coverage Status

Changes Unknown when pulling 861620d on mslw:enh_test_utf8 into ** on duecredit:master**.

coveralls commented Aug 16, 2017

Coverage Status

Changes Unknown when pulling 861620d on mslw:enh_test_utf8 into ** on duecredit:master**.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 16, 2017

Codecov Report

Merging #121 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage   83.27%   83.23%   -0.05%     
==========================================
  Files          44       44              
  Lines        2308     2308              
  Branches      251      251              
==========================================
- Hits         1922     1921       -1     
  Misses        316      316              
- Partials       70       71       +1
Impacted Files Coverage Δ
duecredit/tests/test_io.py 99.39% <100%> (ø) ⬆️
duecredit/io.py 82% <0%> (-0.41%) ⬇️

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 62da28f...861620d. Read the comment docs.

codecov-io commented Aug 16, 2017

Codecov Report

Merging #121 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage   83.27%   83.23%   -0.05%     
==========================================
  Files          44       44              
  Lines        2308     2308              
  Branches      251      251              
==========================================
- Hits         1922     1921       -1     
  Misses        316      316              
- Partials       70       71       +1
Impacted Files Coverage Δ
duecredit/tests/test_io.py 99.39% <100%> (ø) ⬆️
duecredit/io.py 82% <0%> (-0.41%) ⬇️

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 62da28f...861620d. Read the comment docs.

@yarikoptic

This comment has been minimized.

Show comment
Hide comment
@yarikoptic

yarikoptic Aug 16, 2017

Member

since that test indeed should concern itself with testing handling of utf-8, your version is better ;) thus I will merge it. But indeed, as coverage decrease shows, we better also have some dedicated test for import_doi

Member

yarikoptic commented Aug 16, 2017

since that test indeed should concern itself with testing handling of utf-8, your version is better ;) thus I will merge it. But indeed, as coverage decrease shows, we better also have some dedicated test for import_doi

@yarikoptic yarikoptic merged commit 4ea8967 into duecredit:master Aug 16, 2017

5 checks passed

codecov/patch 100% of diff hit (target 83.27%)
Details
codecov/project Absolute coverage decreased by -0.04% but relative coverage increased by +16.72% compared to a35ee08
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 86.338%
Details
@mslw

This comment has been minimized.

Show comment
Hide comment
@mslw

mslw Aug 16, 2017

Contributor

I agree, but I have no idea what exactly should be tested for import_doi. Whether it works with an arbitrary doi (thus failing also when the queried service produces an error, which can be good or bad, as the service can change its API or just have a downtime)? Whether it returns anything? Whether it returns a correct data type (at least for python 2 and unicode entries, unicode object is required for encode/decode to work in other functions)?

Contributor

mslw commented Aug 16, 2017

I agree, but I have no idea what exactly should be tested for import_doi. Whether it works with an arbitrary doi (thus failing also when the queried service produces an error, which can be good or bad, as the service can change its API or just have a downtime)? Whether it returns anything? Whether it returns a correct data type (at least for python 2 and unicode entries, unicode object is required for encode/decode to work in other functions)?

@yarikoptic

This comment has been minimized.

Show comment
Hide comment
@yarikoptic

yarikoptic Aug 16, 2017

Member

Easiest would be to adopt similar to pandas approach which first senses for connection, and if absent, skips the test. Forgot how they called that decorator. And then we could test in a few doi's from different origins

Member

yarikoptic commented Aug 16, 2017

Easiest would be to adopt similar to pandas approach which first senses for connection, and if absent, skips the test. Forgot how they called that decorator. And then we could test in a few doi's from different origins

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment