Skip to content
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

Warning -- warnings.filters was modified by test_warnings #72874

Closed
vstinner opened this issue Nov 14, 2016 · 17 comments
Closed

Warning -- warnings.filters was modified by test_warnings #72874

vstinner opened this issue Nov 14, 2016 · 17 comments
Labels
3.7 (EOL) end of life tests Tests in the Lib/test dir

Comments

@vstinner
Copy link
Member

BPO 28688
Nosy @vstinner, @vadmium, @serhiy-storchaka, @zhangyangyu
Superseder
  • bpo-28727: Implement comparison (x==y and x!=y) for _sre.SRE_Pattern
  • Files
  • warnings_key.patch
  • immortal-filters.patch
  • 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 = <Date 2016-11-22.01:00:20.189>
    created_at = <Date 2016-11-14.12:56:46.432>
    labels = ['3.7', 'tests']
    title = 'Warning -- warnings.filters was modified by test_warnings'
    updated_at = <Date 2016-11-22.08:47:16.244>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2016-11-22.08:47:16.244>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-11-22.01:00:20.189>
    closer = 'martin.panter'
    components = ['Tests']
    creation = <Date 2016-11-14.12:56:46.432>
    creator = 'vstinner'
    dependencies = []
    files = ['45521', '45542']
    hgrepos = []
    issue_num = 28688
    keywords = ['patch']
    message_count = 17.0
    messages = ['280767', '280777', '280782', '280798', '281042', '281047', '281049', '281050', '281079', '281080', '281081', '281082', '281179', '281368', '281371', '281424', '281444']
    nosy_count = 5.0
    nosy_names = ['vstinner', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'xiang.zhang']
    pr_nums = []
    priority = 'normal'
    resolution = 'out of date'
    stage = 'resolved'
    status = 'closed'
    superseder = '28727'
    type = None
    url = 'https://bugs.python.org/issue28688'
    versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

    @vstinner
    Copy link
    Member Author

    The issue bpo-23839 modified the test runner to always clear caches before running tests. As a side effect, test_warnings started to complain with:

    Warning -- warnings.filters was modified by test_warnings

    The issue comes from the following function of test_warnings/init.py:

    def setUpModule():
        py_warnings.onceregistry.clear()
        c_warnings.onceregistry.clear()

    I suggest to rewrite this function as a setUp/tearDown method in BaseTest and *restores* the old value of these dictionaries.

    I guess that the bug affects all Python versions, even if only Python 3.7 logs a warning.

    @vstinner vstinner added 3.7 (EOL) end of life tests Tests in the Lib/test dir labels Nov 14, 2016
    @serhiy-storchaka
    Copy link
    Member

    How you got this warning? I can't reproduce.

    @vstinner
    Copy link
    Member Author

    How you got this warning? I can't reproduce.

    Sorry, I forgot to mention that the warning only occurs if you run Python with -Werror:

    @SELMA$ ./python -Werror -m test test_warnings
    Run tests sequentially
    0:00:00 [1/1] test_warnings
    Warning -- warnings.filters was modified by test_warnings
    test_warnings failed (env changed)

    1 test altered the execution environment:
    test_warnings

    Total duration: 2 sec
    Tests result: SUCCESS

    Moreover, the warning goes away if you don't run tests in verbose mode!?

    haypo@selma$ ./python -Werror -m test -v test_warnings
    ...
    1 test OK.

    Total duration: 2 sec
    Tests result: SUCCESS

    @zhangyangyu
    Copy link
    Member

    test___all__ gets the same behaviour.

    ./python -Werror -m test test___all__
    Run tests sequentially
    0:00:00 [1/1] test___all__
    Warning -- warnings.filters was modified by test___all__
    test___all__ failed (env changed)

    1 test altered the execution environment:
    test___all__

    Total duration: 1 sec
    Tests result: SUCCESS

    And on my PC in company I sometimes get the behaviour for test_distutils.

    I originally thought this is not a problem.

    @vstinner
    Copy link
    Member Author

    Hum, this issue is a regression caused by the issue bpo-23839. The environment warning was already fixed by the issue bpo-18383 (duplicate: issue bpo-26742):

    New changeset f57f4e33ba5e by Martin Panter in branch '3.5':
    Issue bpo-18383: Avoid adding duplicate filters when warnings is reloaded
    https://hg.python.org/cpython/rev/f57f4e33ba5e

    The problem is that _sre.SRE_Pattern doesn't import rich compare: so two patterns are only equal if it's exactly the same object... which is likely when re caches the compiled expression... But the Python test runner now starts by clearing the re cache!

    I see different options:

    • Find something else to not re-initialize warning filters, "_processoptions(sys.warnoptions)" in warnings.py.
    • Fix warnings._add_filter() to implement a custom comparator operator for regular expression objects: compare pattern and flags
    • Implement comparision in _sre.SRE_Pattern

    @vstinner
    Copy link
    Member Author

    • Implement comparision in _sre.SRE_Pattern

    I wrote a patch and opened the issue bpo-28727: "Implement comparison (x==y and x!=y) for _sre.SRE_Pattern".

    @vstinner
    Copy link
    Member Author

    • Fix warnings._add_filter() to implement a custom comparator operator for regular expression objects: compare pattern and flags

    Attached patch warnings_key.patch implements this. I really dislike the patch :-(

    @vstinner
    Copy link
    Member Author

    FYI Python 2.7 is not impacted by this bug because it seems like reimporting warnings.py twice gets a new fresh list from _warnings.filters. I don't undertand how/why.

    @vadmium
    Copy link
    Member

    vadmium commented Nov 18, 2016

    I didn’t really like adding the _add_filter() special handling in the first place, but I went ahead because I couldn’t think of a better way to avoid the problem with reloading the warnings modules. So unless anyone can suggest anything better, I am okay with your patch (even if you dislike it :).

    + return (item[0], _pattern_key(item[1]), item[2], _pattern_key(item[3]))

    The key is based on (action, message, category, module). I think you should add item[4] (lineno).

    @vstinner
    Copy link
    Member Author

    The key is based on (action, message, category, module). I think you should add item[4] (lineno).

    Oops, right!

    @vstinner
    Copy link
    Member Author

    Ok, let me propose a plan for 3.5, 3.6 and 3.7:

    • Remove warnings.filters test on Python 3.5 from regrtest
    • Implement comparison for SRE_Pattern in Python 3.6 and 3.7: issue bpo-28727

    I consider that the issue bpo-28727 is a minor enhancement and so is still good for Python 3.6.

    So we avoid the ugly *warnings_key.patch*.

    Reminder: this issue only occurs in the Python test suite which explicitly reloads modules. It should not occur in the wild.

    @serhiy-storchaka
    Copy link
    Member

    Your plan LGTM.

    @vadmium
    Copy link
    Member

    vadmium commented Nov 18, 2016

    Here is another way to remember that the filter list has already been initialized. I made a new immortal _warnings.filters_initialized flag at the C level. It is actually a list so that it can be mutated and remembered across module reloads, but it is either empty (evaluates as false), or a single element: [True].

    Also, Python 2 does get duplicated filters, but I guess there is not test that exposes it.

    $ python2 -Wall
    . . .
    >>> import warnings
    >>> len(warnings.filters)
    5
    >>> reload(warnings)
    <module 'warnings' from '/usr/lib/python2.7/warnings.pyc'>
    >>> len(warnings.filters)
    6

    I agree there is no need to change Python 2 at this stage.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 21, 2016

    New changeset 75b1091594f8 by Victor Stinner in branch '3.5':
    Issue bpo-28688: Remove warnings.filters check from regrtest
    https://hg.python.org/cpython/rev/75b1091594f8

    New changeset a2616863de06 by Victor Stinner in branch '3.6':
    Issue bpo-28688: Null merge 3.5
    https://hg.python.org/cpython/rev/a2616863de06

    New changeset da042eec6743 by Victor Stinner in branch 'default':
    Issue bpo-28688: Null merge 3.6
    https://hg.python.org/cpython/rev/da042eec6743

    @vstinner
    Copy link
    Member Author

    I implemented x==y operator for _sre.SRE_Pattern in Python 3.6 and 3.7, it fixed this issue.

    For Python 3.5, I removed the warnings.filters test, as we discussed.

    @martin Panter: immortal-filters.patch works because I'm not super excited by the change. Somehow, it looks like a hack... even if I don't see any better solution for Python 3.5.

    Since the bug only impacts Python test suite in the practical, is it really worth it to fix it in Python 3.5 which is almost in the "security fix only" stage?

    @martin: It's up to you, I have no strong opinion on your patch.

    I agree there is no need to change Python 2 at this stage.

    Ok.

    @vadmium
    Copy link
    Member

    vadmium commented Nov 22, 2016

    As long as we are restricted by backwards compatibility, it will be hard to find a hack-free solution. The ideal solution IMO is to re-create _warnings.filters from scratch when _warnings is reloaded, but such a change would be safer only for 3.7.

    So I am happy to leave things as they are. At least until something upsets things again :)

    @vadmium vadmium closed this as completed Nov 22, 2016
    @vstinner
    Copy link
    Member Author

    I'm ok to close this bug :-)

    As long as we are restricted by backwards compatibility, it will be hard to find a hack-free solution. The ideal solution IMO is to re-create _warnings.filters from scratch when _warnings is reloaded, but such a change would be safer only for 3.7.

    Currently it's not possible to "reload" the _warnings module since it
    doesn't use the "module state" API. I don't see how you want to
    trigger an action in the _warnings module itself when it is
    "reloaded". Filteres are used internally in the _warnings C module.

    Maybe we need to enhance the _warnings C module to use the "module
    state" API? (Is it the PEP-489?)

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    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
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants