classification
Title: Update PEP 399 to allow for test discovery
Type: enhancement Stage: resolved
Components: Documentation Versions:
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ezio.melotti Nosy List: brett.cannon, eric.araujo, ezio.melotti, pjenvey, zach.ware
Priority: normal Keywords: patch

Created on 2013-01-01 19:51 by brett.cannon, last changed 2013-01-04 20:27 by ezio.melotti. 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
2013-01-04 20:27:00ezio.melottisetstatus: open -> closed
messages: + msg179077

assignee: eric.araujo -> ezio.melotti
resolution: fixed
stage: commit review -> resolved
2013-01-04 20:20:31brett.cannonsetnosy: + eric.araujo
messages: + msg179076

assignee: eric.araujo
stage: patch review -> commit review
2013-01-04 18:18:13zach.waresetmessages: + msg179063
2013-01-04 18:13:14ezio.melottisetfiles: + issue16835-3.diff

messages: + msg179062
2013-01-03 15:19:33zach.waresetmessages: + msg178948
2013-01-03 04:50:48ezio.melottisetfiles: + issue16835-2.diff
2013-01-03 04:50:39ezio.melottisetfiles: - issue16835-2.diff
2013-01-03 04:49:58ezio.melottisetfiles: + issue16835-2.diff
assignee: ezio.melotti -> (no value)
messages: + msg178911
2013-01-03 03:23:07zach.waresetnosy: + zach.ware
messages: + msg178910
2013-01-02 20:08:29ezio.melottisetfiles: + issue16835.diff
keywords: + patch
messages: + msg178847
2013-01-02 20:06:47pjenveysetnosy: + pjenvey
messages: + msg178845
2013-01-02 00:29:05ezio.melottisetstage: needs patch -> patch review
2013-01-01 20:32:39ezio.melottisetmessages: + msg178751
2013-01-01 19:52:32ezio.melottisetnosy: + ezio.melotti
assignee: ezio.melotti
components: + Documentation
type: enhancement
stage: needs patch
2013-01-01 19:51:45brett.cannoncreate