Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(99412)

#15557: Tests for webbrowser module

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 8 months ago by anton
Modified:
6 years, 7 months ago
Reviewers:
chris.jerdonek, swarmer.pm, rdmurray
CC:
Georg, r.david.murray, asvetlov, cjerdonek, devnull_psf.upfronthosting.co.za, anton.barkovsky
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Patch Set 3 #

Patch Set 4 #

Patch Set 5 #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/test/test_webbrowser.py View 1 2 3 4 1 chunk +192 lines, -0 lines 9 comments Download

Messages

Total messages: 4
cjerdonek
Adding some comments on the latest patch. http://bugs.python.org/review/15557/diff/5935/Lib/test/test_webbrowser.py File Lib/test/test_webbrowser.py (right): http://bugs.python.org/review/15557/diff/5935/Lib/test/test_webbrowser.py#newcode20 Lib/test/test_webbrowser.py:20: class CommandTestMixin: ...
6 years, 7 months ago #1
swarmer.pm_gmail.com
http://bugs.python.org/review/15557/diff/5935/Lib/test/test_webbrowser.py File Lib/test/test_webbrowser.py (right): http://bugs.python.org/review/15557/diff/5935/Lib/test/test_webbrowser.py#newcode20 Lib/test/test_webbrowser.py:20: class CommandTestMixin: On 2012/09/03 18:02:24, cjerdonek wrote: > It ...
6 years, 7 months ago #2
cjerdonek
Replying to one of Anton's comments. http://bugs.python.org/review/15557/diff/5935/Lib/test/test_webbrowser.py File Lib/test/test_webbrowser.py (right): http://bugs.python.org/review/15557/diff/5935/Lib/test/test_webbrowser.py#newcode20 Lib/test/test_webbrowser.py:20: class CommandTestMixin: On ...
6 years, 7 months ago #3
r.david.murray
6 years, 7 months ago #4
On 2012/09/03 18:45:08, anton.barkovsky wrote:
> http://bugs.python.org/review/15557/diff/5935/Lib/test/test_webbrowser.py
> File Lib/test/test_webbrowser.py (right):
> 
>
http://bugs.python.org/review/15557/diff/5935/Lib/test/test_webbrowser.py#new...
> Lib/test/test_webbrowser.py:20: class CommandTestMixin:
> On 2012/09/03 18:02:24, cjerdonek wrote:
> > It seems like "MethodTestMixin" might be a better name since from the
caller's
> > perspective this class is for testing calls to different methods on a web
> > browser instance.
> 
> This mixin is used by all *CommandTests and only by them, so I think the
current
> name really fits. If anything, I'd make it a subclass of TestCase and call it
> CommandTestCase.

Well, making it a subclass of TestCase doesn't work in the general case, because
then unittest will try to run it as a test case.  In this example that would not
cause test failures, since there are no test_ methods on the mixin, but the
general pattern in the Python test suite is to have these Mixin test classes
(often called xxxxBase instead of Mixin, for historical reasons).

> 
>
http://bugs.python.org/review/15557/diff/5935/Lib/test/test_webbrowser.py#new...
> Lib/test/test_webbrowser.py:30: sequence order to whatever is left over after
> removing the options.
> On 2012/09/03 18:02:25, cjerdonek wrote:
> > Is it necessary to test things this way (position independent and comparing
to
> > what is left over)?  It seems like this might miss things like whether an
> option
> > argument is correctly following its corresponding option flag.  In any case,
> it
> > might be worth a sentence saying why this approach is being done (e.g. if
> there
> > is a limit to what you can guarantee). 
> 
> On the one hand, currently there are no "context-sensitive" arguments and
> checking them will probably require using argparse. On the other hand if they
> appear one day I'm not sure if it's possible to adapt _test without breaking
all
> the tests.

Yes, that's a problem.  I made it work with the current set of commands we are
dealing with, but it may break if things get more complex.  But, I'm going to
leave that for someone else to deal with then.  Using argparse is an interesting
suggestion.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+