This issue tracker has been migrated to GitHub ,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2013年01月01日 19:51 by brett.cannon, last changed 2022年04月11日 14:57 by admin. This issue is now closed.
| Files | ||||
|---|---|---|---|---|
| File name | Uploaded | Description | Edit | |
| issue16835.diff | ezio.melotti, 2013年01月02日 20:08 | |||
| issue16835-2.diff | ezio.melotti, 2013年01月03日 04:50 | |||
| issue16835-3.diff | ezio.melotti, 2013年01月04日 18:13 | |||
| Messages (11) | |||
|---|---|---|---|
| msg178748 - (view) | Author: Brett Cannon (brett.cannon) * (Python committer) | Date: 2013年01月01日 19:51 | |
Don't have the base tests inherit from TestCase else they will be discovered by unittest and run even though they are not fully defined. See http://bugs.python.org/issue16748 as the trigger for this issue. |
|||
| msg178751 - (view) | Author: Ezio Melotti (ezio.melotti) * (Python committer) | Date: 2013年01月01日 20:32 | |
Here's a patch: 1) the base class doesn't inherit from TestCase anymore -- the subclasses do; 2) added a skipUnless() decorator on the C subclass; 3) used the modern if __name__ == '__main__': unittest.main() idiom; 4) renamed the AcceleratedExampleTest to CExampleTest (I prefer the PyFoo/CFoo parallel, "Accelerated" doesn't necessarily imply that it's testing the C version); 5) added a paragraph to explain the idiom; 6) added Post-History; 7) did a couple of minor cleanup in the code; |
|||
| msg178845 - (view) | Author: Philip Jenvey (pjenvey) * (Python committer) | Date: 2013年01月02日 20:06 | |
Hey Ezio, you forgot to attach the patch |
|||
| msg178847 - (view) | Author: Ezio Melotti (ezio.melotti) * (Python committer) | Date: 2013年01月02日 20:08 | |
That would explain why no one was reviewing it :) |
|||
| msg178910 - (view) | Author: Zachary Ware (zach.ware) * (Python committer) | Date: 2013年01月03日 03:23 | |
A few minor grammatical and style nits in the prose added at the end of the diff: > +The test module defines a base class with the tests methods that access the > +``heapq`` module through a ``self.heapq`` class attribute, and two subclasses > +that set this attribute to either the Python and C versions of the module. I think that should be "Python or C version" (and -> or; versions -> version). > +Note that only the two subclasses inherit from ``unittest.TestCase`` -- this > +prevents the ``ExampleTest`` class to be detected as a ``TestCase`` subclass "to be" should be "from being", I believe. > +by ``unittest`` test discovery. > +A ``skipUnless`` decorator can be added to the class that tests the C code This should either have another newline inbetween or be reflowed. Either one paragraph or two makes sense to me, but I can't tell which way you actually meant it to go which makes for confusing reading of the ReST source. As far as the example code changes, those look good to me :) |
|||
| msg178911 - (view) | Author: Ezio Melotti (ezio.melotti) * (Python committer) | Date: 2013年01月03日 04:49 | |
Thanks for the review, new patch attached. > This should either have another newline inbetween or be reflowed. > Either one paragraph or two makes sense to me, but I can't tell which > way you actually meant I meant one and half :) I guess in HTML that would have been a <br> inside the <p>, but I don't think it really matters if it's rendered as a single paragraph. |
|||
| msg178948 - (view) | Author: Zachary Ware (zach.ware) * (Python committer) | Date: 2013年01月03日 15:19 | |
> Thanks for the review, new patch attached. You're quite welcome. Is there anything I've missed in the process of reviewing itself? This is the first time I've reviewed a patch here... I did miss another nit in the prose, though; "the tests methods" in the first line isn't quite right, but I can't decide if it should be "the test's methods" (singular possessive), "the tests' methods" (plural possessive), or "test methods" (non-specific, non-possessive). Any of the three that better gets your point across makes me happy :) >> This should either have another newline inbetween or be reflowed. >> Either one paragraph or two makes sense to me, but I can't tell which >> way you actually meant > > I meant one and half :) > I guess in HTML that would have been a <br> inside the <p>, but I don't > think it really matters if it's rendered as a single paragraph. I see. As is works for me if it works for you, though I might lean towards rounding it up to 2 paragraphs :) |
|||
| msg179062 - (view) | Author: Ezio Melotti (ezio.melotti) * (Python committer) | Date: 2013年01月04日 18:13 | |
Attached a new patch. |
|||
| msg179063 - (view) | Author: Zachary Ware (zach.ware) * (Python committer) | Date: 2013年01月04日 18:18 | |
Looks good to me :) |
|||
| msg179076 - (view) | Author: Brett Cannon (brett.cannon) * (Python committer) | Date: 2013年01月04日 20:20 | |
LGTM as well. Feel free to commit it, Ezio, or assign to me and I will commit it later (probably this weekend). |
|||
| msg179077 - (view) | Author: Ezio Melotti (ezio.melotti) * (Python committer) | Date: 2013年01月04日 20:27 | |
Done in http://hg.python.org/peps/rev/3740f42d3b94, thanks for the reviews! |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022年04月11日 14:57:40 | admin | set | github: 61039 |
| 2013年01月04日 20:27:00 | ezio.melotti | set | status: open -> closed messages: + msg179077 assignee: eric.araujo -> ezio.melotti resolution: fixed stage: commit review -> resolved |
| 2013年01月04日 20:20:31 | brett.cannon | set | nosy:
+ eric.araujo messages: + msg179076 assignee: eric.araujo stage: patch review -> commit review |
| 2013年01月04日 18:18:13 | zach.ware | set | messages: + msg179063 |
| 2013年01月04日 18:13:14 | ezio.melotti | set | files:
+ issue16835-3.diff messages: + msg179062 |
| 2013年01月03日 15:19:33 | zach.ware | set | messages: + msg178948 |
| 2013年01月03日 04:50:48 | ezio.melotti | set | files: + issue16835-2.diff |
| 2013年01月03日 04:50:39 | ezio.melotti | set | files: - issue16835-2.diff |
| 2013年01月03日 04:49:58 | ezio.melotti | set | files:
+ issue16835-2.diff assignee: ezio.melotti -> (no value) messages: + msg178911 |
| 2013年01月03日 03:23:07 | zach.ware | set | nosy:
+ zach.ware messages: + msg178910 |
| 2013年01月02日 20:08:29 | ezio.melotti | set | files:
+ issue16835.diff keywords: + patch messages: + msg178847 |
| 2013年01月02日 20:06:47 | pjenvey | set | nosy:
+ pjenvey messages: + msg178845 |
| 2013年01月02日 00:29:05 | ezio.melotti | set | stage: needs patch -> patch review |
| 2013年01月01日 20:32:39 | ezio.melotti | set | messages: + msg178751 |
| 2013年01月01日 19:52:32 | ezio.melotti | set | nosy:
+ ezio.melotti assignee: ezio.melotti components: + Documentation type: enhancement stage: needs patch |
| 2013年01月01日 19:51:45 | brett.cannon | create | |