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.

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

Jump to bottom

webbrowser._synthesize uses outdated calling signature for webbrowser.register #75197

Closed
jmsdvl mannequin opened this issue Jul 24, 2017 · 13 comments
Closed

webbrowser._synthesize uses outdated calling signature for webbrowser.register #75197

jmsdvl mannequin opened this issue Jul 24, 2017 · 13 comments
Assignees
@serhiy-storchaka
Labels
3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@jmsdvl
Copy link
Mannequin

jmsdvl mannequin commented Jul 24, 2017

BPO 31014
Nosy @ncoghlan, @vstinner, @serhiy-storchaka, @davesteele, @jmsdvl, @miss-islington
PRs
  • bpo-31014: webbrowser._synthesize calls webbrowser.register with outdated signature #2689
  • bpo-31014: Fix the webbrowser module. #7267
  • [3.7] bpo-31014: Fix the webbrowser module. (GH-7267) #8183
  • [3.7] bpo-35308: Fix regression where BROWSER env var is not respected. (GH-10693) #10729
    		•

    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 = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2018-07-08.08:19:09.179>
created_at = <Date 2017-07-24.14:32:24.484>
labels = ['3.7', '3.8', 'type-bug', 'library']
title = 'webbrowser._synthesize uses outdated calling signature for webbrowser.register'
updated_at = <Date 2018-11-26.21:49:33.945>
user = 'https://github.com/jmsdvl'

    bugs.python.org fields:

    activity = <Date 2018-11-26.21:49:33.945>
actor = 'miss-islington'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2018-07-08.08:19:09.179>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2017-07-24.14:32:24.484>
creator = 'jmsdvl'
dependencies = []
files = []
hgrepos = []
issue_num = 31014
keywords = ['patch']
message_count = 13.0
messages = ['298976', '298986', '298987', '298989', '299000', '299007', '318173', '318174', '318243', '321266', '321268', '330466', '330469']
nosy_count = 6.0
nosy_names = ['ncoghlan', 'vstinner', 'serhiy.storchaka', 'daves', 'jmsdvl', 'miss-islington']
pr_nums = ['2689', '7267', '8183', '10729']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue31014'
versions = ['Python 3.7', 'Python 3.8']
    @jmsdvl
    Copy link
    Mannequin Author

    jmsdvl mannequin commented Jul 24, 2017

    The function register of the webbrowser module was updated a little while back to use the newish keyword-only argument syntax; however, the function _synthesize (also in webbrowser) is still using the outdated positional arguments only calling convention; leading a pair of tests in [Lib/tests/test_webbrowser.py](https://github.com/python/cpython/blob/main/Lib/tests/test_webbrowser.py) to fail. I've issued a PR that fixes the function call to use the more modern calling convention.

    @jmsdvl jmsdvl mannequin added 3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Jul 24, 2017
    @serhiy-storchaka
    Copy link
    Member

    Is it possible to write a test?

    @jmsdvl
    Copy link
    Mannequin Author

    jmsdvl mannequin commented Jul 24, 2017

    What would a new test be testing? I only found this behaviour because two existing tests were failing (with the PR they pass, of course). I'm happy to write a test, I'm just not sure what the test should be testing that the existing tests don't already test.

    @serhiy-storchaka
    Copy link
    Member

    These tests are not failing on our buildbots and developer's computers. Seems they are failing only in special environment. This path of execution is not tested in more common environments, therefore possible regression couldn't be caught until somebody run tests in similar special environment. New test should test this path of execution in common environments.

    @jmsdvl
    Copy link
    Mannequin Author

    jmsdvl mannequin commented Jul 24, 2017

    Ok I understand what you mean.

    I think the only conditions for test failure are (A) $BROWSER is set and (B) $BROWSER is set to an actual executable that should be caught by ``register_standard_browsers`` prior to execution reaching the last if clause of ``register_standard_browsers`` (``if 'BROWSER' in os.environ:``) - those two conditions should be sufficient for execution to reach the offending line of code.

    I'll see if I can cook up a test for that. I think the right way to do this is to monkeypatch os.environ and shutil.which, am I right?

    @serhiy-storchaka
    Copy link
    Member

    This would be not easy. Since the BROWSER environment variable should be patched before the first use the webbrowser module, the test should import webbrowser in a separate process (you can use the test.support.script_helper.assert_python_ok() helper).

    _synthesize() is used in two places with different arguments: in register_standard_browsers() and in get().

    @vstinner
    Copy link
    Member

    bpo-33693 has been marked as a duplicate of this issue. It seems to be a regression caused by bpo-24241.

    @vstinner
    Copy link
    Member

    I'm surprised that test_webbrowser pass on my Fedora 28, but it fails for Greg Walters:
    https://bugs.python.org/issue33693#msg318163

    Some tests are skipped depending on the OS? Maybe we should mock more things, or something else, to get a better code coverage.

    I ran PR 2689 new test without the fix on webbrowser.py: the test doesn't fail. So this PR is not enough to detect bpo-33693.

    @serhiy-storchaka
    Copy link
    Member

    PR 7267 is based on PR 2689, but adds two tests that cover both cases of using webbrowser._synthesize().

    @serhiy-storchaka serhiy-storchaka added the 3.8 only security fixes label May 31, 2018
    @serhiy-storchaka serhiy-storchaka self-assigned this May 31, 2018
    @serhiy-storchaka
    Copy link
    Member

    New changeset 25b804a by Serhiy Storchaka in branch 'master':
    bpo-31014: Fix the webbrowser module. (GH-7267)
    25b804a

    @miss-islington
    Copy link
    Contributor

    New changeset a410f9f by Miss Islington (bot) in branch '3.7':
    bpo-31014: Fix the webbrowser module. (GH-7267)
    a410f9f

    @serhiy-storchaka
    Copy link
    Member

    New changeset 8c281ed by Serhiy Storchaka (Zhiming Wang) in branch 'master':
    bpo-35308: Fix regression where BROWSER env var is not respected. (GH-10693)
    8c281ed

    @miss-islington
    Copy link
    Contributor

    New changeset 2a37f01 by Miss Islington (bot) in branch '3.7':
    bpo-35308: Fix regression where BROWSER env var is not respected. (GH-10693)
    2a37f01

    @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
    Assignees

    @serhiy-storchaka serhiy-storchaka

    Labels
    3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @vstinner @serhiy-storchaka @miss-islington