Skip to content

Fix Python2 test failures in certain locales #22213

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

Merged
merged 2 commits into from
Aug 13, 2018
Merged

Fix Python2 test failures in certain locales #22213

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a0d727
Fix Python2 test failures in certain locales
ginggs Aug 6, 2018
6cceebc
Move tests into TestLocaleUtils class
ginggs Aug 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions pandas/tests/util/test_util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,32 @@ def test_numpy_errstate_is_default():
assert np.geterr() == expected


@td.skip_if_windows
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

def test_can_set_locale_valid_set():
# Setting the default locale should return True
assert tm.can_set_locale('') is True


@td.skip_if_windows
def test_can_set_locale_invalid_set():
# Setting an invalid locale should return False
assert tm.can_set_locale('non-existent_locale') is False


@td.skip_if_windows
def test_can_set_locale_invalid_get(monkeypatch):
# In some cases, an invalid locale can be set,
# but a subsequent getlocale() raises a ValueError
# See GH 22129

def mockgetlocale():
raise ValueError()

with monkeypatch.context() as m:
m.setattr(locale, 'getlocale', mockgetlocale)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 (?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

assert tm.can_set_locale('') is False


@td.skip_if_windows
class TestLocaleUtils(object):

Expand Down
19 changes: 8 additions & 11 deletions pandas/util/testing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,23 +504,19 @@ def set_locale(new_locale, lc_var=locale.LC_ALL):

try:
locale.setlocale(lc_var, new_locale)

try:
normalized_locale = locale.getlocale()
except ValueError:
yield new_locale
normalized_locale = locale.getlocale()
if com._all_not_none(*normalized_locale):
yield '.'.join(normalized_locale)
else:
if com._all_not_none(*normalized_locale):
yield '.'.join(normalized_locale)
else:
yield new_locale
yield new_locale
finally:
locale.setlocale(lc_var, current_locale)


def can_set_locale(lc, lc_var=locale.LC_ALL):
"""
Check to see if we can set a locale without raising an Exception.
Check to see if we can set a locale, and subsequently get the locale,
without raising an Exception.

Parameters
----------
Expand All @@ -538,7 +534,8 @@ def can_set_locale(lc, lc_var=locale.LC_ALL):
try:
with set_locale(lc, lc_var=lc_var):
pass
except locale.Error: # horrible name for a Exception subclass
except (ValueError,
locale.Error): # horrible name for a Exception subclass
return False
else:
return True
Expand Down