Fix Python2 test failures in certain locales #22213

ginggs · 2018-08-06T10:52:14Z

Check that we can also get the locale, after setting it, without raising an Exception.

closes Python2 test_datetime_name_accessors failure with some locales #22129
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2018-08-06T10:52:16Z

Hello @ginggs! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on August 11, 2018 at 08:35 Hours UTC

gfyoung · 2018-08-06T13:24:08Z

https://travis-ci.org/pandas-dev/pandas/jobs/412589280#L3432

@ginggs : Good start! Looks like you have some lint issues though.

codecov · 2018-08-06T14:08:59Z

Codecov Report

Merging #22213 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #22213      +/-   ##
==========================================
+ Coverage   92.08%   92.08%   +<.01%     
==========================================
  Files         169      169              
  Lines       50694    50691       -3     
==========================================
- Hits        46682    46681       -1     
+ Misses       4012     4010       -2

Flag	Coverage Δ
#multiple	`90.49% <100%> (ø)`	⬆️
#single	`42.34% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/util/testing.py	`86.06% <100%> (+0.16%)`	⬆️
pandas/core/arrays/categorical.py	`95.75% <0%> (ø)`	⬆️

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 0370740...6cceebc. Read the comment docs.

WillAyd

Need to add test cases for this

ginggs · 2018-08-08T13:24:05Z

I don't know how we can test this, since this change is making a test in pandas more robust against things external to pandas.

WillAyd · 2018-08-08T15:46:43Z

Should be able to monkey patch getlocale:

https://docs.pytest.org/en/latest/monkeypatch.html

ginggs · 2018-08-08T16:38:00Z

Thanks! I'll see what I can do.

TomAugspurger · 2018-08-08T18:20:43Z

Hmm, I seem to have reproduced this in my branch migrating us to circleci 2.0: https://circleci.com/gh/pandas-dev/pandas/16656?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link (search for unknown locale).

ginggs · 2018-08-09T07:07:32Z

@TomAugspurger it seems you have a Debian Stretch container there, and the locales-all package is installed.

I'm still struggling to come up with a test case that is not too contrived.

ginggs · 2018-08-10T11:05:57Z

@WillAyd I've added two test cases, and verified that the second test fails without my changes to pandas/util/testing.py.

TomAugspurger · 2018-08-10T13:01:54Z

These tests look good to me. Merging later today unless @WillAyd has other comments.

WillAyd

One minor comment

WillAyd · 2018-08-10T15:29:50Z

pandas/tests/util/test_util.py

+    def mockgetlocale():
+        raise ValueError()
+
+    monkeypatch.setattr(locale, 'setlocale', mocksetlocale)


Can you use the context manager for patching instead? From the monkey patch docs it is the preferred watching of patching stdlib

ginggs · 2018-08-10T17:55:26Z

@WillAyd updated using monkeypatch.context().

WillAyd

Context manager looks good. One question on what is being patched

ginggs · 2018-08-10T18:35:04Z

@WillAyd sorry, what is the question?

WillAyd

Trying again - not sure what happened to my comment earlier

WillAyd · 2018-08-10T18:37:03Z

pandas/tests/util/test_util.py

+
+    with monkeypatch.context() as m:
+        m.setattr(locale, 'setlocale', mocksetlocale)
+        m.setattr(locale, 'getlocale', mockgetlocale)


Do we need to patch this? From the comment above, shouldn't it naturally raise a ValueError after setting non-existent_locale as the locale?

If locale.Error is raised while setting non-existent_locale, then the locale is not changed, and 'getlocale' will return the current locale.

The second test needs to simulate successfully setting non-existent_locale and raising a ValueError so we can test the changes in pandas/util/testing.py.

Hmm so does non-existent_locale fail when getting or setting? Reason I ask is because I think there's a better way to isolate the expected behavior here, i.e. we should only need to mock the getter or setter but not both within a given test

non-existent_locale always fails when setting.

In the second test I am trying to simulate the behaviour of en_IL or ak_GH or one of several others in locales-all, which can be set, but then raise a ValueError when getlocale is called.

Right - so isn't the patch to setlocale unnecessary? So long as getlocale raises (as your patch causes to happen) the set behavior is more or less irrelevant (?)

On my system (Ubuntu 18.04), Python2 gives me:

>>> import locale; locale.getlocale() (None, None)

Whereas Python3 gives me:

>>> import locale; locale.getlocale() ('en_ZA', 'UTF-8')

This works (in Python2 and Python3):

>>> locale.setlocale(locale.LC_ALL, '') 'en_ZA.UTF-8'

So shall we rather go with assert tm.can_set_locale('') is False and drop the monkeypatch on setlocale?

Are you saying tm.can_set_locale(locale.getlocale()) isn't working across both versions? I believe a tuple is a valid argument. Would be cleaner to just send that value through instead of the empty string

The documentation gives this an example.
https://docs.python.org/2/library/locale.html

import locale locale.setlocale(locale.LC_ALL, '')

Ah OK that's fair - thanks for pointing there!

Check that we can also get the locale, after setting it, without raising an Exception. Closes: #22129

ginggs · 2018-08-11T08:42:03Z

@WillAyd reworked to drop the monkeypatch on setlocale, and added another test.

WillAyd

FYI if you merge in your updates instead of rebasing in the future it will maintain the review messages, making this process a little easier

WillAyd · 2018-08-11T16:28:55Z

pandas/tests/util/test_util.py

@@ -417,6 +417,32 @@ def test_numpy_errstate_is_default():
    assert np.geterr() == expected


+@td.skip_if_windows


Have these skips always been in place or is this a new addition in latest push?

They've been there since I first added the tests.

OK thanks. Can you move these into the TestLocaleUtils class? It seems like they should logically be grouped in there and that class already has the decorator to skip windows.

Should be the last change - thanks for hanging with the updates!

OK, will do. Unfortunately I'll only be able to get to it tomorrow.

WillAyd

Lgtm @TomAugspurger merge away if satisfied

WillAyd · 2018-08-13T18:24:31Z

Thanks @ginggs!

TomAugspurger · 2018-08-13T19:11:20Z

Note, `monkeypatch.context` is new in pytest 3.6: https://docs.pytest.org/en/latest/changelog.html#pytest-3-6-0-2018-05-23 We should update our minimum pytest in `ci/environment-dev.yaml`

…

On Mon, Aug 13, 2018 at 1:24 PM William Ayd ***@***.***> wrote: Thanks @ginggs <https://github.com/ginggs>! — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#22213 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIueJ9BQA6ZZJsO6UuBsrvq9YwD9dks5uQcRlgaJpZM4VwLDD> .

WillAyd · 2018-08-13T19:18:01Z

@TomAugspurger good point as that was discussed in #21531 (review) can submit a PR later today to bump

ginggs · 2018-08-13T21:01:20Z

@WillAyd you are welcome, and thanks for your patience!

WillAyd · 2018-08-14T15:49:16Z

Just looking back here we probably want a whatsnew as well - @ginggs do you want to do that in a separate PR? Should be able to leverage the same Development section I am creating in #22320

ginggs · 2018-08-14T17:23:28Z

@WillAyd sure, I can do that. Do I wait until #22320 is merged?

WillAyd · 2018-08-14T18:48:14Z

Probably easiest that way. Hope to have that merged in soon - thanks!

Backported from pandas-devgh-21739. Backport of pandas-devgh-22213.

* 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.

* Fix Python2 test failures in certain locales Check that we can also get the locale, after setting it, without raising an Exception. Closes: pandas-dev#22129

gfyoung added the 2/3 Compat label Aug 6, 2018

WillAyd requested changes Aug 6, 2018

View reviewed changes

WillAyd requested changes Aug 10, 2018

View reviewed changes

WillAyd reviewed Aug 10, 2018

View reviewed changes

Fix Python2 test failures in certain locales

8a0d727

Check that we can also get the locale, after setting it, without raising an Exception. Closes: #22129

WillAyd reviewed Aug 11, 2018

View reviewed changes

Move tests into TestLocaleUtils class

6cceebc

WillAyd approved these changes Aug 12, 2018

View reviewed changes

TomAugspurger merged commit ab274c2 into pandas-dev:master Aug 13, 2018

ginggs deleted the patch-1 branch August 13, 2018 05:54

TomAugspurger mentioned this pull request Aug 13, 2018

CI: Migrate to circleci 2.0 #21814

Merged

WillAyd mentioned this pull request Aug 13, 2018

Bump Min pytest Version #22319

Closed

TomAugspurger added this to the 0.24.0 milestone Sep 10, 2018

TomAugspurger mentioned this pull request Sep 10, 2018

CI / BLD: Various CI Backports #22637

Merged

gfyoung added a commit to forking-repos/pandas that referenced this pull request Sep 11, 2018

TST: Patch locale handling

c780f95

Backported from pandas-devgh-21739. Backport of pandas-devgh-22213.

WillAyd mentioned this pull request Nov 13, 2018

pandas 0.23.4 fails unit tests #23638

Closed

		@@ -417,6 +417,32 @@ def test_numpy_errstate_is_default():
		assert np.geterr() == expected


		@td.skip_if_windows

Fix Python2 test failures in certain locales #22213

Fix Python2 test failures in certain locales #22213

Conversation

ginggs commented Aug 6, 2018 • edited Loading

pep8speaks commented Aug 6, 2018 • edited Loading

Comment last updated on August 11, 2018 at 08:35 Hours UTC

gfyoung commented Aug 6, 2018

codecov bot commented Aug 6, 2018 • edited Loading

Codecov Report

WillAyd left a comment

Choose a reason for hiding this comment

ginggs commented Aug 8, 2018

WillAyd commented Aug 8, 2018

ginggs commented Aug 8, 2018

TomAugspurger commented Aug 8, 2018

ginggs commented Aug 9, 2018

ginggs commented Aug 10, 2018

TomAugspurger commented Aug 10, 2018

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ginggs commented Aug 10, 2018

WillAyd left a comment

Choose a reason for hiding this comment

ginggs commented Aug 10, 2018

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ginggs commented Aug 11, 2018

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd left a comment

Choose a reason for hiding this comment

WillAyd commented Aug 13, 2018

TomAugspurger commented Aug 13, 2018 via email

WillAyd commented Aug 13, 2018

ginggs commented Aug 13, 2018

WillAyd commented Aug 14, 2018

ginggs commented Aug 14, 2018

WillAyd commented Aug 14, 2018

ginggs commented Aug 6, 2018 •

edited

Loading

pep8speaks commented Aug 6, 2018 •

edited

Loading

codecov bot commented Aug 6, 2018 •

edited

Loading