bpo-31014: `webbrowser._synthesize` calls `webbrowser.register` with outdated signature #2689

ghost · 2017-07-13T05:38:35Z

Tonight while running the test suite I noticed the following two errors with the tests for the webbrowser module:

======================================================================
ERROR: test_get (test.test_webbrowser.ImportTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jms/projects/forks/cpython/Lib/test/test_webbrowser.py", line 290, in test_get
    webbrowser.get('fakebrowser')
  File "/home/jms/projects/forks/cpython/Lib/webbrowser.py", line 42, in get
    register_standard_browsers()
  File "/home/jms/projects/forks/cpython/Lib/webbrowser.py", line 567, in register_standard_browsers
    cmd = _synthesize(cmdline, -1)
  File "/home/jms/projects/forks/cpython/Lib/webbrowser.py", line 116, in _synthesize
    register(browser, None, controller, update_tryorder)
TypeError: register() takes from 2 to 3 positional arguments but 4 were given

======================================================================
ERROR: test_register (test.test_webbrowser.ImportTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jms/projects/forks/cpython/Lib/test/test_webbrowser.py", line 277, in test_register
    webbrowser.register('Example1', ExampleBrowser)
  File "/home/jms/projects/forks/cpython/Lib/webbrowser.py", line 26, in register
    register_standard_browsers()
  File "/home/jms/projects/forks/cpython/Lib/webbrowser.py", line 567, in register_standard_browsers
    cmd = _synthesize(cmdline, -1)
  File "/home/jms/projects/forks/cpython/Lib/webbrowser.py", line 116, in _synthesize
    register(browser, None, controller, update_tryorder)
TypeError: register() takes from 2 to 3 positional arguments but 4 were given

----------------------------------------------------------------------

After a little bit of digging I discovered that webbrowser_synthesize is using an old call signature for webbrowser.register. This PR should fix that. I believe this is the only place the change in function signature was either affected or not updated.

https://bugs.python.org/issue31014

the-knights-who-say-ni · 2017-07-13T05:38:37Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

mention-bot · 2017-07-13T05:38:38Z

@jmsdvl, thanks for your PR! By analyzing the history of the files in this pull request, we identified @birkenfeld, @freddrake and @serhiy-storchaka to be potential reviewers.

serhiy-storchaka · 2017-07-24T17:50:24Z

Lib/webbrowser.py

@@ -113,7 +113,7 @@ def _synthesize(browser, update_tryorder=1):
        controller = copy.copy(controller)
        controller.name = browser
        controller.basename = os.path.basename(browser)
-        register(browser, None, controller, update_tryorder)
+        register(browser, None, instance=controller, preferred=update_tryorder)


update_tryorder is 1 or -1. preferred is a boolean parameter. 1 and -1 both are true in boolean context.

I think that the update_tryorder parameter of _synthesize() should be replaced with boolean preferred as in register() (see bpo-24241).

Update tryorder is now a boolean as requested.

brettcannon · 2018-02-02T22:04:50Z

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

ghost · 2018-04-05T02:31:36Z

Lib/webbrowser.py

@@ -113,7 +113,7 @@ def _synthesize(browser, update_tryorder=1):
        controller = copy.copy(controller)
        controller.name = browser
        controller.basename = os.path.basename(browser)
-        register(browser, None, controller, update_tryorder)
+        register(browser, None, instance=controller, preferred=update_tryorder)


Update tryorder is now a boolean as requested.

ghost · 2018-04-05T02:34:27Z

Lib/test/test_webbrowser.py

@@ -265,6 +267,23 @@ def test_register_default(self):
    def test_register_preferred(self):
        self._check_registration(preferred=True)

+    def test_environment_variable_BROWSER_is_respected(self):


This test does accurately catch the regression as far as I can tell but it seems a little obscure. Thoughts?

ghost · 2018-04-05T02:35:01Z

I have made the requested changes; please review again

bedevere-bot · 2018-04-05T02:35:04Z

Thanks for making the requested changes!

@serhiy-storchaka: please review the changes made to this pull request.

serhiy-storchaka · 2018-05-16T13:26:36Z

I tried to write a less obscure test for the bug in register_standard_browsers(), but failed. But I think we can use simpler test for the original bug in _synthesize().

    def test_synthesize(self):
        webbrowser = support.import_fresh_module('webbrowser')
        name = os.path.basename(sys.executable).lower()
        webbrowser.register(name, None, webbrowser.GenericBrowser(name))
        webbrowser.get(sys.executable)

ncoghlan · 2018-05-30T12:40:52Z

Lib/test/test_webbrowser.py

+            def mock_which(path):
+                return True
+
+            class Test(test_webbrowser.ImportTest):


I almost missed that this re-ran the ImportTest suite with BROWSER set.

I think that's OK as a test case, but it would be clearer as part of a dedicated BrowserOverrideTest class that's placed after ImportTest (and mentions in a class level comment that it re-runs the ImportTest cases in a subprocess with BROWSER set.

serhiy-storchaka · 2018-07-08T08:20:11Z

Merged as #7267 with other tests.

Co-Authored-By: Makdon <[email protected]>

Co-Authored-By: Makdon <[email protected]> (cherry picked from commit e6557d3) Co-authored-by: Xtreak <[email protected]>

Co-Authored-By: Makdon <[email protected]>

fixes: _synthesize called register with the wrong signature

995cdac

the-knights-who-say-ni added the CLA not signed label Jul 13, 2017

ghost changed the title ~~fixes: _synthesize called register with the wrong signature~~ trivial-fixes: _synthesize called register with the wrong signature Jul 13, 2017

ghost changed the title ~~trivial-fixes: _synthesize called register with the wrong signature~~ bpo-31014 webbrowser._synthesize calls webbrowser.register with outdated signature Jul 24, 2017

serhiy-storchaka requested changes Jul 24, 2017

View reviewed changes

serhiy-storchaka added type-bug An unexpected behavior, bug, or error and removed CLA not signed labels Jul 24, 2017

the-knights-who-say-ni added the CLA signed label Jul 24, 2017

brettcannon added the awaiting changes label Feb 2, 2018

John Still added 3 commits April 4, 2018 20:08

Merge branch 'master' into fix-webbrowser-_synthesize

271b8ef

Change update_tryorder to a boolean

e83fb70

Regression test for webbrowser with BROWSER set

e00858e

ghost commented Apr 5, 2018

View reviewed changes

bedevere-bot added awaiting change review and removed awaiting changes labels Apr 5, 2018

serhiy-storchaka self-assigned this May 13, 2018

serhiy-storchaka changed the title ~~bpo-31014 webbrowser._synthesize calls webbrowser.register with outdated signature~~ bpo-31014: webbrowser._synthesize calls webbrowser.register with outdated signature May 13, 2018

ncoghlan reviewed May 30, 2018

View reviewed changes

serhiy-storchaka mentioned this pull request May 31, 2018

bpo-31014: Fix the webbrowser module. #7267

Merged

serhiy-storchaka closed this Jul 8, 2018

serhiy-storchaka removed their assignment Dec 5, 2018

benjaminp pushed a commit that referenced this pull request Sep 11, 2019

bpo-36528: Remove duplicate re tests. (GH-2689)

e6557d3

Co-Authored-By: Makdon <[email protected]>

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 11, 2019

bpo-36528: Remove duplicate re tests. (pythonGH-2689)

0f92eb6

Co-Authored-By: Makdon <[email protected]> (cherry picked from commit e6557d3) Co-authored-by: Xtreak <[email protected]>

miss-islington added a commit that referenced this pull request Sep 11, 2019

bpo-36528: Remove duplicate re tests. (GH-2689)

f90cbcb

Co-Authored-By: Makdon <[email protected]> (cherry picked from commit e6557d3) Co-authored-by: Xtreak <[email protected]>

DinoV pushed a commit to DinoV/cpython that referenced this pull request Sep 12, 2019

bpo-36528: Remove duplicate re tests. (pythonGH-2689)

1ada6df

Co-Authored-By: Makdon <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-31014: `webbrowser._synthesize` calls `webbrowser.register` with outdated signature #2689

bpo-31014: `webbrowser._synthesize` calls `webbrowser.register` with outdated signature #2689

ghost commented Jul 13, 2017 •

edited by ghost

Loading

the-knights-who-say-ni commented Jul 13, 2017

mention-bot commented Jul 13, 2017

serhiy-storchaka Jul 24, 2017 •

edited

Loading

ghost Apr 5, 2018

brettcannon commented Feb 2, 2018

ghost Apr 5, 2018

ghost Apr 5, 2018

ghost commented Apr 5, 2018

bedevere-bot commented Apr 5, 2018

serhiy-storchaka commented May 16, 2018

ncoghlan May 30, 2018

serhiy-storchaka commented Jul 8, 2018

bpo-31014: webbrowser._synthesize calls webbrowser.register with outdated signature #2689

bpo-31014: webbrowser._synthesize calls webbrowser.register with outdated signature #2689

Conversation

ghost commented Jul 13, 2017 • edited by ghost Loading

the-knights-who-say-ni commented Jul 13, 2017

mention-bot commented Jul 13, 2017

serhiy-storchaka Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

ghost Apr 5, 2018

Choose a reason for hiding this comment

brettcannon commented Feb 2, 2018

ghost Apr 5, 2018

Choose a reason for hiding this comment

ghost Apr 5, 2018

Choose a reason for hiding this comment

ghost commented Apr 5, 2018

bedevere-bot commented Apr 5, 2018

serhiy-storchaka commented May 16, 2018

ncoghlan May 30, 2018

Choose a reason for hiding this comment

serhiy-storchaka commented Jul 8, 2018

bpo-31014: `webbrowser._synthesize` calls `webbrowser.register` with outdated signature #2689

bpo-31014: `webbrowser._synthesize` calls `webbrowser.register` with outdated signature #2689

ghost commented Jul 13, 2017 •

edited by ghost

Loading

serhiy-storchaka Jul 24, 2017 •

edited

Loading