homepage

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.

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 

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.

Thanks for the review, I've updated the patch.

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.

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.

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.

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.

I think you forgot to write `test_open_with_autoraise_false` for Chrome tests.

I did indeed.

I added some comments on the latest patch on the review tool.

New changeset 5da3b2df38b3 by R David Murray in branch 'default':
#15557,#15447,#15509: webbrowser test suite added.
http://hg.python.org/cpython/rev/5da3b2df38b3 

Thanks, Anton. And thank you Chris for the initial reviews.

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.

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.

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'.

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.

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.