Skip to content

Fix text locale test issue on Windows#901

Merged
corranwebster merged 8 commits into
mainfrom
fix/text-locale-test
Mar 10, 2022
Merged

Fix text locale test issue on Windows#901
corranwebster merged 8 commits into
mainfrom
fix/text-locale-test

Conversation

@corranwebster
Copy link
Copy Markdown
Contributor

This attempts to set the locale to the current value before changing to the testing locale to ensure that a reset will work. On failure, it skips which matches previous behaviour.

Fixes #899

This attempts to set the locale to the current value before changing to the
testing locale to ensure that a reset will work.  On failure, it skips which
matches previous behaviour.
Comment thread kiva/tests/agg/test_text.py Outdated
@rahulporuri
Copy link
Copy Markdown
Contributor

With the changes in this PR, the test is still being skipped. Looking into this further, it looks like this call is what is raising the locale.Error

with locale_context(locale.LC_CTYPE, ("en", "ASCII")):

and it looks like there is something fundamentally wrong with the test now -

>>> import locale
>>> locale.getlocale(locale.LC_CTYPE)
(None, None)
>>> locale.setlocale(locale.LC_CTYPE, ('en', 'ASCII'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\rporuri\.edm\envs\enable-test-3.6-wx\lib\locale.py", line 598, in setlocale
    return _setlocale(category, locale)
locale.Error: unsupported locale setting
>>> locale.setlocale(locale.LC_CTYPE, ('en', 'UTF-8'))
'en_US.UTF-8'

Copy link
Copy Markdown
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

It looks like there are issues with locale.getlocale/locale.setlocale on windows so let's just explicitly skip the test on windows instead of indirectly skipping them in this way. Ref https://bugs.python.org/issue38324

@corranwebster
Copy link
Copy Markdown
Contributor Author

Did the locale test work on anything other than mac?

@rahulporuri
Copy link
Copy Markdown
Contributor

Did the locale test work on anything other than mac?

ha. looks like @rkern introduced the try/except because locale setting was failing on linux - 4abc500

@corranwebster
Copy link
Copy Markdown
Contributor Author

Possibly we should be using ("C", "ASCII")? Still doesn't work on windows.

@rahulporuri
Copy link
Copy Markdown
Contributor

rahulporuri commented Mar 9, 2022

Possibly we should be using ("C", "ASCII")? Still doesn't work on windows.

Hey that actually works on Python 3.8.10 and 3.6.13 -

>>> import locale
>>> locale.setlocale(locale.LC_CTYPE, ('C', 'ASCII'))
'C'

I'm not sure if it still conveys the original intent of the test though.

@corranwebster
Copy link
Copy Markdown
Contributor Author

Trying now with all skipping turned off - I think the ("en", "UTF-8") will still fail on windows, but there may be an equivalent locale setting we can use.

I think it's the ASCII vs UTF-8 stuff that it's testing, rather than the language part of the setting, but it might be useful to have @rkern check that it's OK.

@corranwebster
Copy link
Copy Markdown
Contributor Author

corranwebster commented Mar 9, 2022

Aha - ('en', 'utf8') works on Windows.

Edit: and on mac and linux as well.

@corranwebster corranwebster requested a review from rkern March 9, 2022 12:47
Copy link
Copy Markdown
Member

@rkern rkern left a comment

Choose a reason for hiding this comment

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

LGTM. Yeah, the code being "tested" here is locale-independent by construction, so this test more records intent than anything else; it's testing that we removed the old implementation. The language is indeed an unimportant part. I think I chose C_ASCII POSIX requires it. Some Linux installations might remove en locale data.

I think I would reintroduce the try: except: around both setlocale() calls just to safeguard things for non-Windows platforms that don't have en locales. If this test fails due to one of the setlocale()s (the "given" or "cleanup" parts of the test), we should not hold up the test suite. This is a nice-to-have test, but not really critical.

@corranwebster corranwebster merged commit 62f390f into main Mar 10, 2022
@jwiggins jwiggins deleted the fix/text-locale-test branch March 15, 2022 19:22
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.

Regression with locale tests on Windows/Wx

3 participants