Skip to content

CLN/TST: Cleanup tests in test_ujson.py #21739

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 5, 2018
Merged

CLN/TST: Cleanup tests in test_ujson.py #21739

merged 1 commit into from
Jul 5, 2018

Conversation

gfyoung
Copy link
Member

@gfyoung gfyoung commented Jul 5, 2018

  • Actually test the outputs
  • Add more parameterization
  • Take advantage of pytest features, such as exception contexts

This test module really needed an update 😄

@gfyoung gfyoung added Testing pandas testing functions or related to the test suite IO JSON read_json, to_json, json_normalize Clean labels Jul 5, 2018
@gfyoung gfyoung added this to the 0.24.0 milestone Jul 5, 2018
@gfyoung gfyoung requested review from WillAyd and jreback July 5, 2018 00:48
jreback
jreback reviewed Jul 5, 2018
View reviewed changes
pandas/tests/io/json/test_ujson.py Outdated
import locale
savedlocale = locale.getlocale(locale.LC_NUMERIC)
saved_locale = locale.getlocale(locale.LC_NUMERIC)

try:
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we have a contest manager for locale setting

Copy link
Member Author

@gfyoung gfyoung Jul 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

  • Good point, but it doesn't do the Exception checking that I think this test wants to do.
  • I think I would want to tap into _can_set_locale instead. Any reason why I shouldn't utilize into this private method?

Copy link
Contributor

Choose a reason for hiding this comment

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

not familiar with that
but happy to use existing code (so u know why it’s private?)

Copy link
Member Author

Choose a reason for hiding this comment

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

(so u know why it’s private?)

No clue 😄 , but I'll go ahead and make it public and use it instead.

@gfyoung gfyoung force-pushed the parametrize-more-tests branch from 06ae3da to d5f0f03 Compare July 5, 2018 04:34
@codecov
Copy link

codecov bot commented Jul 5, 2018

Codecov Report

Merging #21739 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #21739   +/-   ##
=======================================
  Coverage   91.92%   91.92%           
=======================================
  Files         158      158           
  Lines       49705    49705           
=======================================
  Hits        45693    45693           
  Misses       4012     4012
Flag Coverage Δ
#multiple 90.3% <ø> (ø) ⬆️
#single 41.99% <ø> (ø) ⬆️

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 1070976...d5f0f03. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 5, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@3f7a385). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #21739   +/-   ##
=========================================
  Coverage          ?   91.93%           
=========================================
  Files             ?      159           
  Lines             ?    49719           
  Branches          ?        0           
=========================================
  Hits              ?    45707           
  Misses            ?     4012           
  Partials          ?        0
Flag Coverage Δ
#multiple 90.3% <100%> (?)
#single 42.02% <100%> (?)
Impacted Files Coverage Δ
pandas/util/testing.py 85.33% <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 3f7a385...30eb1fb. Read the comment docs.

@gfyoung gfyoung force-pushed the parametrize-more-tests branch 3 times, most recently from a43d9b6 to 528d3d2 Compare July 5, 2018 10:09
@gfyoung
CLN/TST: Cleanup tests in test_ujson.py
30eb1fb 
* Actually test the outputs
* Add more parametrization
* Take advantage of pytest features,
such as exception contexts
@gfyoung gfyoung force-pushed the parametrize-more-tests branch from 528d3d2 to 30eb1fb Compare July 5, 2018 16:48
@gfyoung
Copy link
Member Author

gfyoung commented Jul 5, 2018

@jreback : Everything is green. PTAL.

@jreback jreback merged commit c7e4b21 into pandas-dev:master Jul 5, 2018
@jreback
Copy link
Contributor

jreback commented Jul 5, 2018

thanks @gfyoung nice cleanups! keep em coming!

@gfyoung gfyoung deleted the parametrize-more-tests branch July 5, 2018 22:38
gfyoung added a commit to forking-repos/pandas that referenced this pull request Sep 8, 2018
@gfyoung
TST: Propagate lc_var in can_set_locale
10774ee 
Backported from pandas-devgh-21739.
gfyoung added a commit to forking-repos/pandas that referenced this pull request Sep 11, 2018
@gfyoung
TST: Patch locale handling
c780f95 
Backported from pandas-devgh-21739.
Backport of pandas-devgh-22213.
gfyoung added a commit that referenced this pull request Sep 11, 2018
@gfyoung
CI / BLD: Various CI Backports (#22637)
183e92f 
* CI: Bump NumPy to 1.9.3

Backport of gh-22499.

* BLD: Fix openpyxl to 2.5.5

Backport of gh-22601.

* CI: Resolve timeout issue on Travis

Backported from gh-22429.

* CI: Migrate to CircleCI 2.0

Backport of gh-21814.

* Upgrade Cython to >=0.28.2

Backported from gh-21688.

* TST: Patch locale handling

Backported from gh-21739.
Backport of gh-22213.
@gfyoung gfyoung mentioned this pull request Sep 11, 2018
Merged
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
@gfyoung
CLN/TST: Cleanup tests in test_ujson.py (pandas-dev#21739)
ac1cbb1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback left review comments

@WillAyd WillAyd Awaiting requested review from WillAyd

Assignees
No one assigned
Labels
Clean IO JSON read_json, to_json, json_normalize Testing pandas testing functions or related to the test suite
Projects
None yet
Milestone
0.24.0
Development

Successfully merging this pull request may close these issues.
2 participants
@gfyoung @jreback