classification
Title: Tests for webbrowser module
Type: Stage: resolved
Components: Tests Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: 15447 15509 Superseder:
Assigned To: Nosy List: anton.barkovsky, asvetlov, chris.jerdonek, georg.brandl, python-dev, r.david.murray
Priority: normal Keywords: patch

Created on 2012-08-04 17:52 by anton.barkovsky, last changed 2012-09-03 17:41 by chris.jerdonek. This issue is now closed.

Files
File name Uploaded Description Edit
test_webbrowser.patch anton.barkovsky, 2012-08-04 17:52 review
test_webbrowser_v2.patch anton.barkovsky, 2012-08-12 14:34 review
test_webbrowser_v3.patch anton.barkovsky, 2012-08-12 19:09 review
test_webbrowser.patch r.david.murray, 2012-09-03 02:45 review
test_webbrowser.patch r.david.murray, 2012-09-03 15:00 review
Messages (18)
msg167423 - (view) Author: Anton Barkovsky (anton.barkovsky) * Date: 2012-08-04 17:52
Attaching a patch with some tests for webbrowser module.

The tests fail unless #15509 is fixed.
They also print lots of warnings unless #15447 is fixed.
msg167983 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-08-11 17:43
It seems like these tests can be made more DRY.  For example, the line `args = popen.cmd_line` appears in every test.  You could make an assert_args() helper method to address this.  There also seems to be a lot of overlap between tests for each browser.  A DRY approach would let one see more easily how the tests differ across browsers.

Do you also need to include the test boilerplate at the bottom?  See, for example--

http://hg.python.org/cpython/file/867de88b69f0/Lib/test/test_textwrap.py#l732
msg168020 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-08-12 04:36
Regrtest has evolved.  For new test files (ie: 3.3 and later) the preferred (at least by me :) idiom now is just:

   if __name__ == '__main__':
       unittest.main()

Everything else is now automatic, using the unittest machinery.  For an example, see test_smptd.
msg168041 - (view) Author: Anton Barkovsky (anton.barkovsky) * Date: 2012-08-12 14:34
Thanks for the review, I've updated the patch.
msg168052 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-08-12 16:30
It still seems like things could be made more DRY.  Also, the pattern of having assert_unix_browser() execute various function blocks depending on whether various arguments are not None doesn't seem as clean or scalable as it should be (e.g. if the number of assert statements were to grow).

What about defining helper methods like check_open(), check_open_new_tab(), and check_open_new(), and then having the various test_<browser>() methods call each of them as appropriate?  For example--

+        browser.open_new('http://www.example.com/')
+        args = popen.cmd_line
+        self.assertEqual(args, ['test', '-raise', '-remote',
+            'openURL(http://www.example.com/,new-window)'])

could become a call to check_open_new().  The helper methods could default to the most common string arguments so that you will only need to define and pass the string arguments when the browser uses strings that are different from the defaults.
msg168060 - (view) Author: Anton Barkovsky (anton.barkovsky) * Date: 2012-08-12 19:09
Updated, added separate helper methods and 'test' and 'http://www.example.com/' are no longer hardcoded.

> The helper methods could default to the most common string arguments so that you will only need to define and pass the string arguments when the browser uses strings that are different from the defaults.

I don't see a good reason to provide defaults. If some browsers have same arguments it's just a coincidence, the arguments are not likely to ever change,  and I don't like when a test case is scattered all over the module in the form of defaults.
msg168064 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-08-12 19:56
Thanks, Anton.  It is looking a lot better now.  I still have comments, but because my comments have not been on the substance of the patch and because I am not a core developer, I will defer to others at this point.
msg169738 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-09-03 02:45
Thanks, Anton.

I took your last patch just a bit further, mostly to make it easy to break up the test methods that test multiple things into test methods that test just one thing.  I also made the test insensitive to the order of the options on the command line, since in theory that shouldn't matter (this makes the tests more robust in the face of changes to the code). 

I'm pretty sure I transcribed all the tests correctly, but it would be great if you would double check me.
msg169761 - (view) Author: Anton Barkovsky (anton.barkovsky) * Date: 2012-09-03 13:13
I think you forgot to write `test_open_with_autoraise_false` for Chrome tests.
msg169766 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-09-03 15:00
I did indeed.
msg169768 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-09-03 16:03
I added some comments on the latest patch on the review tool.
msg169775 - (view) Author: Roundup Robot (python-dev) Date: 2012-09-03 16:53
New changeset 5da3b2df38b3 by R David Murray in branch 'default':
#15557,#15447,#15509: webbrowser test suite added.
http://hg.python.org/cpython/rev/5da3b2df38b3
msg169780 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-09-03 16:56
Thanks, Anton.  And thank you Chris for the initial reviews.
msg169784 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-09-03 17:24
Hmm.  For some reason I did not get emailed these review comments, and I did not see your note before I did the checkin.  I will take a look.
msg169785 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-09-03 17:33
Thanks, David.  I wasn't sure if you had seen the comments.  They're mostly stylistic, though, so it's not too big of a deal.
msg169786 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-09-03 17:36
Yeah, you make some good points, but I think I may already have spent more time on this that is justified by the amount of usage webbrowser gets :)  So I think I'm going to leave it as is, as being 'good enough'.
msg169787 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-09-03 17:39
Oh, I see.  I did get the email, it's just that my email filter put it into a different folder from what I was expected.
msg169788 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-09-03 17:41
Fair enough. :)  I may keep a couple of those changes in mind if I ever have a chance to visit this module myself in the future.
History
Date User Action Args
2012-09-03 17:41:49chris.jerdoneksetmessages: + msg169788
2012-09-03 17:39:10r.david.murraysetmessages: + msg169787
2012-09-03 17:36:20r.david.murraysetmessages: + msg169786
2012-09-03 17:33:56chris.jerdoneksetmessages: + msg169785
2012-09-03 17:24:33r.david.murraysetmessages: + msg169784
2012-09-03 16:56:25r.david.murraysetstatus: open -> closed
resolution: fixed
messages: + msg169780

stage: resolved
2012-09-03 16:53:26python-devsetnosy: + python-dev
messages: + msg169775
2012-09-03 16:03:13chris.jerdoneksetmessages: + msg169768
2012-09-03 15:00:27r.david.murraysetfiles: + test_webbrowser.patch

messages: + msg169766
2012-09-03 13:13:21anton.barkovskysetmessages: + msg169761
2012-09-03 02:45:46r.david.murraysetfiles: + test_webbrowser.patch

messages: + msg169738
2012-08-13 13:00:29asvetlovsetnosy: + asvetlov
2012-08-12 19:56:24chris.jerdoneksetmessages: + msg168064
2012-08-12 19:09:41anton.barkovskysetfiles: + test_webbrowser_v3.patch

messages: + msg168060
2012-08-12 16:30:19chris.jerdoneksetmessages: + msg168052
2012-08-12 14:34:30anton.barkovskysetfiles: + test_webbrowser_v2.patch

messages: + msg168041
2012-08-12 04:36:22r.david.murraysetmessages: + msg168020
2012-08-11 17:43:49chris.jerdoneksetnosy: + chris.jerdonek
messages: + msg167983
2012-08-11 16:15:34pitrousetnosy: + georg.brandl
2012-08-09 13:22:11asvetlovsetdependencies: + A file is not properly closed by webbrowser._invoke, UnixBrowser.open sometimes passes zero-length argument to the browser.
versions: + Python 3.4, - Python 3.3
2012-08-04 17:52:57anton.barkovskycreate