classification
Title: test_gdbm.py converted to unittest
Type: enhancement Stage:
Components: Tests Versions: Python 2.6
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: brett.cannon, giampaolo.rodola
Priority: normal Keywords: patch

Created on 2008-01-28 23:31 by giampaolo.rodola, last changed 2008-03-13 21:03 by brett.cannon. This issue is now closed.

Files
File name Uploaded Description Edit
test_gdbm.diff giampaolo.rodola, 2008-01-28 23:31 test_gdbm.py converted to unittest
test_gdbm.py giampaolo.rodola, 2008-01-28 23:31 test_gdbm.py converted to unittest (modified .py file)
test_gdbm.diff giampaolo.rodola, 2008-01-30 02:51 Refined version (diff)
test_gdbm.py giampaolo.rodola, 2008-01-30 02:52 Refined version (py)
test_gdbm.diff giampaolo.rodola, 2008-02-06 18:56 Diff file generated via svn diff
Messages (9)
msg61806 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2008-01-28 23:31
In attachment. All existent tests are unchanged.
msg61807 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-01-29 00:08
Have a look at http://code.google.com/p/google-highly-open-participation-psf/issues/detail?id=290
where a GHOP student did a conversion as well. Any chance to come up
with a possible merged version that takes the best from your work,
Giampaolo and the student's work?
msg61808 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2008-01-29 01:03
> Have a look at
http://code.google.com/p/google-highly-open-participation-psf/issues/detail?id=290
> where a GHOP student did a conversion as well.

Interesting, didn't know about that.
In future I'll check that site before start working on a new patch
involving the test suite since it seems those students are already
working on these kind of issues.

> Any chance to come up with a possible merged version 
> that takes the best from your work, Giampaolo and the
> student's work?

Don't know... the suite is extremely simple and, IMO, I don't even think
there's a real need to do that.
I noticed that the student added/changed some stuff like the new
"test_modes" method which maybe could be useful to have.
msg61810 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-01-29 01:24
On Jan 28, 2008 5:03 PM, Giampaolo Rodola' <report@bugs.python.org> wrote:
>
> Giampaolo Rodola' added the comment:
>
> > Have a look at
> http://code.google.com/p/google-highly-open-participation-psf/issues/detail?id=290
> > where a GHOP student did a conversion as well.
>
> Interesting, didn't know about that.
> In future I'll check that site before start working on a new patch
> involving the test suite since it seems those students are already
> working on these kind of issues.
>

We have held off on committing their code until we get contributor
agreements from them. Hopefully that will all get resolved in February
and thus you won't have to check the GHOP site for long.

> > Any chance to come up with a possible merged version
> > that takes the best from your work, Giampaolo and the
> > student's work?
>
> Don't know... the suite is extremely simple and, IMO, I don't even think
> there's a real need to do that.
> I noticed that the student added/changed some stuff like the new
> "test_modes" method which maybe could be useful to have.

Well, if you have an opinion, feel free to leave a comment in this
issue about it. I will most likely be the one who does the checkin and
I will read this issue before I commit.
msg61811 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2008-01-29 02:20
> Well, if you have an opinion, feel free to leave a comment in this
> issue about it. I will most likely be the one who does the checkin and
> I will read this issue before I commit.

 * One of the things I dislike is the fact that the student used "self.g
= gdbm.open(self.filename, 'c')" in setUp method since if that fails
there will be a NameError exception raised in tearDown method and the
errors reported by unittest would be 2 where in fact it would be only 1.
Probably a thing like this one would have been better:

    def setUp(self):
        self.g = None
    
    def tearDown(self):
        if self.g is not None:
            self.g.close()
	  ...

    def test_it(self):
        self.g = gdbm.open(self.filename, 'c')
        ...

- Another thing I don't like is that os.unlink(self.filename) executed
in tearDown would be better if protected by a try/except statement.

- +1 for the self.g.close() used by the student in the tearDown method.
That was a thing I haven't considered and it's not included in my patch.

- +0.5 for the "test_modes" method added by the student. Maybe it's
useful, maybe it's not, I don't know. 


Aside from that I don't see other relevant differences since the code is
really minimalistic. Feel free to commit the patch you consider to be
the best. 
If you want me to do so I can provide a merged version of my patch
including the differences I described above.
msg61813 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-01-29 03:14
On Jan 28, 2008 6:20 PM, Giampaolo Rodola' <report@bugs.python.org> wrote:
>
> Giampaolo Rodola' added the comment:
>
> > Well, if you have an opinion, feel free to leave a comment in this
> > issue about it. I will most likely be the one who does the checkin and
> > I will read this issue before I commit.
>
>  * One of the things I dislike is the fact that the student used "self.g
> = gdbm.open(self.filename, 'c')" in setUp method since if that fails
> there will be a NameError exception raised in tearDown method and the
> errors reported by unittest would be 2 where in fact it would be only 1.
> Probably a thing like this one would have been better:
>
>     def setUp(self):
>         self.g = None
>
>     def tearDown(self):
>         if self.g is not None:
>             self.g.close()
>           ...
>
>     def test_it(self):
>         self.g = gdbm.open(self.filename, 'c')
>         ...
>

Could also still open perform the open() call in setUp() and do what
you need in the tearDown() to not be an error. There should probably
also be a way to not cause an error if the file path is just not
writable as that is not a sign of a failing test but an unavailable
resource.

> - Another thing I don't like is that os.unlink(self.filename) executed
> in tearDown would be better if protected by a try/except statement.
>

Even better is test.test_support.unlink() which handles those details for you.

> - +1 for the self.g.close() used by the student in the tearDown method.
> That was a thing I haven't considered and it's not included in my patch.
>
> - +0.5 for the "test_modes" method added by the student. Maybe it's
> useful, maybe it's not, I don't know.
>
>
> Aside from that I don't see other relevant differences since the code is
> really minimalistic. Feel free to commit the patch you consider to be
> the best.
> If you want me to do so I can provide a merged version of my patch
> including the differences I described above.

Fine by me. Or you can wait until I have time to look at your code.
It's up to you.
msg61839 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2008-01-30 02:51
Updated version in attachment. 
Changes:

- Added "if self.g is not None: self.g.close()" clause in tearDown.
- Used "test.test_support.unlink(filename)" instead of the try/except
statement.
- Added tests for the flag clause in the open() statement by trying all
supported mode flags. I also called the method "test_flags" instead of
"test_modes" since "mode" is a kwarg for open().
- Added tests for trying to open non existing database with flag == 'r'
and 'w'.

Untested methods which are now tested:
- keys().
- firstkey().
- nextkey().
- reorganize()

The only method currently not tested yet is sync(). I'm not sure how to
test it, maybe I'm misunderstanding its purpose.
msg62115 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2008-02-06 18:56
The diff file in attachment was generated by using TortoiseSVN and it
seems to be different than using "svn diff".
The new diff file is in attachment.
msg63515 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-03-13 21:03
Committed in revision 61374 (w/ changes so that the key stuff is not
order-dependent). Thanks, Giampaolo!
History
Date User Action Args
2008-03-13 21:03:09brett.cannonsetstatus: open -> closed
resolution: accepted
messages: + msg63515
2008-02-17 22:10:15brett.cannonsetassignee: brett.cannon
2008-02-08 18:59:02christian.heimessetpriority: normal
keywords: + patch
2008-02-06 18:56:55giampaolo.rodolasetfiles: + test_gdbm.diff
messages: + msg62115
2008-01-30 02:52:02giampaolo.rodolasetfiles: + test_gdbm.py
2008-01-30 02:51:32giampaolo.rodolasetfiles: + test_gdbm.diff
messages: + msg61839
2008-01-29 03:14:31brett.cannonsetmessages: + msg61813
2008-01-29 02:20:16giampaolo.rodolasetmessages: + msg61811
2008-01-29 01:24:35brett.cannonsetmessages: + msg61810
2008-01-29 01:03:27giampaolo.rodolasetmessages: + msg61808
2008-01-29 00:08:22brett.cannonsetnosy: + brett.cannon
messages: + msg61807
2008-01-28 23:31:20giampaolo.rodolasetfiles: + test_gdbm.py
2008-01-28 23:31:00giampaolo.rodolacreate