Skip to content

test_c_locale_coercion fails when the default LC_CTYPE != "C" #76183

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

Closed
embray opened this issue Nov 10, 2017 · 12 comments
Closed

test_c_locale_coercion fails when the default LC_CTYPE != "C" #76183

embray opened this issue Nov 10, 2017 · 12 comments
Labels
3.7 (EOL) end of life tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Comments

@embray
Copy link
Contributor

embray commented Nov 10, 2017

BPO 32002
Nosy @ncoghlan, @xdegaye, @embray
PRs
  • gh-76183: Fix legacy locale coercion tests on platforms that already have a default C.UTF-8 locale #4361
  • bpo-32002: Refactor C locale coercion tests #4369
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = None
    created_at = <Date 2017-11-10.10:47:52.200>
    labels = ['3.7', 'type-bug', 'tests']
    title = 'test_c_locale_coercion fails when the default LC_CTYPE != "C"'
    updated_at = <Date 2020-06-28.07:34:48.407>
    user = 'https://github.com/embray'

    bugs.python.org fields:

    activity = <Date 2020-06-28.07:34:48.407>
    actor = 'ncoghlan'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Tests']
    creation = <Date 2017-11-10.10:47:52.200>
    creator = 'erik.bray'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 32002
    keywords = ['patch']
    message_count = 11.0
    messages = ['306019', '306022', '306023', '306027', '306036', '306078', '306079', '306083', '307785', '308466', '372506']
    nosy_count = 3.0
    nosy_names = ['ncoghlan', 'xdegaye', 'erik.bray']
    pr_nums = ['4361', '4369']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue32002'
    versions = ['Python 3.7']

    @embray
    Copy link
    Contributor Author

    embray commented Nov 10, 2017

    Several of the tests in test_c_locale_coercion (particularly LocaleCoercionTests._check_c_locale_coercion) tend to assume that the system default locale used when setting setlocale(category, "") and when all the relevant environment variables are empty/blank will be the "C"/"POSIX" locale.

    While this is often true POSIX does not require this to be the case. For example on Cygwin it already defaults to "C.UTF-8", so these tests fail because they assume the legacy coercion will be used, when it isn't (e.g. the LC_CTYPE environment variable does not get forced to "C.UTF-8"). In principle this can affect any platform, however, that chooses a different default.

    @embray embray added 3.7 (EOL) end of life tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error labels Nov 10, 2017
    @ncoghlan
    Copy link
    Contributor

    bpo-30672 is potentially related here - some of the test cases are already disabled on Mac OS X and other *BSD systems since the tests assume that C & POSIX are aliases of each other.

    I've also added Xavier to the nosy list, since the current implementation and tests aren't quite right for Android either and it would be good to come up with a unified solution to more robust platform feature detection: https://bugs.python.org/issue28180#msg305850

    @embray
    Copy link
    Contributor Author

    embray commented Nov 10, 2017

    Yes, I looked at some of the other issues pertaining to this, but it wasn't immediately apparent how to kill multiple birds with one stone, so here I just focused on this one assumption.

    @ncoghlan
    Copy link
    Contributor

    OK, I'd been meaning to get back to refactoring those tests anyway, so assigning this to myself.

    I'm thinking that the right way to go will be to give the test case a more explicit model of "expected platform behaviour" (initialised in setupModule), rather than having that be implicit in a bunch of conditionals scattered throughout the individual test cases.

    Then we'd have at least the following cases:

    • default is C, POSIX is an alias for C (most Linux distros)
    • default is C, POSIX is a separate locale (*BSD)
    • default is C.UTF-8 (Cygwin, potentially Android depending on exactly how we resolve that)

    @ncoghlan ncoghlan self-assigned this Nov 10, 2017
    @embray
    Copy link
    Contributor Author

    embray commented Nov 10, 2017

    In my PR there's a behavior test for the default, so we don't have to hard-code that on a per-platform basis at least. The C != POSIX thing I'm not sure you can easily test for.

    @ncoghlan
    Copy link
    Contributor

    The essential problem in both this issue and bpo-30672 is that the tests are currently incorporating some Linux-specific assumptions about ways to request the "C" locale.

    In #4369, I've taken the approach of making the baseline tests only cover "C" and "invalid.ascii", and then explicitly *opt-in* to testing an empty locale and "POSIX" on Linux machines.

    If that's enough to get the test passing on Cygwin, I'm inclined to leave it at that. Dynamically calculated test expectations always make me nervous, since it's all too easy to end up with bugs that impact both the test case and the expectation calculator in the same way, and hence end up with the test passing when it should really fail.

    @ncoghlan
    Copy link
    Contributor

    Note: I'm not entirely sold on my own argument though, as I believe at least Alpine Linux already interprets the empty locale as C.UTF-8, so it may make more sense to use your dynamic check with both the empty string and "POSIX", and only testing those locales if they get reported back as effectively configuring the "C" locale.

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Nov 11, 2017

    Several of the tests in test_c_locale_coercion (particularly LocaleCoercionTests._check_c_locale_coercion) tend to assume that the system default locale used when setting setlocale(category, "") and when all the relevant environment variables are empty/blank will be the "C"/"POSIX" locale.

    While this is often true POSIX does not require this to be the case.

    I think you are right. The section starting with "The values of locale categories shall be determined by a precedence order;" in [1] states:

    1. If the LANG environment variable is not set or is set to the empty string, the implementation-defined default locale shall be used.

    In the current implementation of PR 4334 [2] only one change to test_c_locale_coercion is needed to fix the failures of some subtests of test_PYTHONCOERCECLOCALE_set_to_warn when all the locale envt variables are set to the empty string. All the other tests are unchanged and ok because the new _Py_SetLocaleFromEnv() function [3] causes Android to behave as a plain *nix platform except when the locale envt variables are unset or set to an empty string.

    [1] http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap08.html
    [2] PR 4334: Fix the implementation of PEP-538 on Android
    [3] And because after calling setlocale(category, "C"), setlocale(category) returns "C" on Android (this may not be the case on Cygwin).

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Dec 7, 2017

    For the POSIX case, we're going to fix the implementation to always handle that the same way as it does the "C" locale: https://bugs.python.org/issue30672#msg307784

    So the main question to address with the refactoring here will be capturing the expected behaviour for the 'locale setting is an empty string' case.

    @ncoghlan
    Copy link
    Contributor

    New changeset 9c19b02 by Nick Coghlan in branch 'master':
    bpo-32002: Refactor C locale coercion tests (GH-4369)
    9c19b02

    @ncoghlan
    Copy link
    Contributor

    Removing issue assignment, as I'm no longer actively investigating this.

    @erlend-aasland
    Copy link
    Contributor

    Closing, as the OP has not responded after multiple pings on the linked PR.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
    Projects
    Development

    No branches or pull requests

    3 participants