Ezio, Thanks for the review it was excellent. I got most of them fixed and will post a new version soon. http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py File Lib/test/test_os.py (right): http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode55 Lib/test/test_os.py:55: # Tests creating TESTFN and os.rename On 2013/03/04 18:15:16, ezio.melotti wrote:

I think it would be better to make a separate class for os.rename. The changes to setUp/tearDown are only needed for the new tests, but they are executed for all the old tests too, and that's not necessary. I agree I only put it in the FileTests class because there was a single test that already existing in that class. A new class will make it much cleaner, good suggestion. Will be implemented in next version.

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode62 Lib/test/test_os.py:62: self.rename_src_file = "%s_%d_tmp" % ("test_rename_src_file", \ On 2013/03/04 18:15:16, ezio.melotti wrote:

All the \ are useless and should be removed (or possibly replaced by an extra set of ()). I thought those were required but you are right they are not needed so I removed.

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode85 Lib/test/test_os.py:85: if (os.path.exists(self.rename_dst_directory + "/file.txt")): On 2013/03/04 18:15:16, ezio.melotti wrote:

Shouldn't this use os.path.join()? Also "/file.txt" is repeated over and over -- it would be better to use a variable instead. I used os.path.join() and I made a variable all good suggestions. Thanks!

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode125 Lib/test/test_os.py:125: os.rmdir(self.rename_src_directory) On 2013/03/04 18:15:16, ezio.melotti wrote:

This could be rewritten as: files = [support.TESTFN, self.rename_src_file, ...] for file in files: if os.path.exists(file): os.unlink(file)

for the dirs you can have the same 4 lines with rmdir instead, or check if they are files or dir and use the same loop. Done.....this is a terrific way to perform the same functions but with fewer lines of code. Thank you.

Why are this cpython_only? Does the behavior depend on the CPython implementation or on the OS and/or underlying posix function? Nope, the original test case had this so I included it. But if you

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode161 Lib/test/test_os.py:161: @support.cpython_only On 2013/03/04 18:15:16, ezio.melotti wrote: think it is ok I removed it. http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode164 Lib/test/test_os.py:164: (sys.platform == 'win32') ), \ On 2013/03/04 18:15:16, ezio.melotti wrote:

All the \ should be removed, and there are also 4 sets of () that are not necessary and could be removed as well. Ok I removed both the \ and the (). I personally think the () make code clearer.

Is this because you only tested it on these platforms? I would run it on other platforms too, and possibly add specific skips if they fail there (unless of course the failure is caused by a bug that should be fixed). If the skip is really necessary I would create a new skip decorator and use that, instead of repeating this over and over. I only have access to Mac OS X, Linux, and Windows but would be glad to

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode165 Lib/test/test_os.py:165: 'test specific to windows or linux or darwin only') On 2013/03/04 18:15:16, ezio.melotti wrote: try on other platforms if I can get access. I removed this code for now. http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode167 Lib/test/test_os.py:167: if ((sys.platform == 'darwin') or (sys.platform.startswith == "linux")): On 2013/03/04 18:15:16, ezio.melotti wrote:

All the () are unnecessary and should be removed. Done....sorry about that just a personal preference.

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode171 Lib/test/test_os.py:171: [b"beans", b"bacon", b"eggs", b"spam"]) On 2013/03/04 18:15:16, ezio.melotti wrote:

PEP8 says that this should either be indented as

self.assertEqual(fobj.read().splitlines(), [b"beans", b"bacon", b"eggs", b"spam"])

or as

self.assertEqual( fobj.read().splitlines(), [b"beans", b"bacon", b"eggs", b"spam"])

http://www.python.org/dev/peps/pep-0008/#indentation Yep that is taken care of in the next version.

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode185 Lib/test/test_os.py:185: os.unlink(self.rename_dst_file) # delete rename_dst_file On 2013/03/04 18:15:16, ezio.melotti wrote:

Shouldn't the setUp/tearDown already take care of this? setUp/teadDown does in most cases. But for cases when destination doesn't exist the destination file must be removed.

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode369 Lib/test/test_os.py:369: self.assertRaises(OSError, os.rename, self.rename_src_file, \ On 2013/03/04 18:15:16, ezio.melotti wrote:

self.assertRaises() can be used as a context manager too, to make the code more readable. Ummmm....I will have to research context manager.

http://bugs.python.org/review/16278/diff/7518/Lib/test/test_os.py#newcode396 Lib/test/test_os.py:396: self.assertEqual(os.path.exists(self.rename_dst_directory), True) On 2013/03/04 18:15:16, ezio.melotti wrote:

self.assertTrue(os.path.exists(self.rename_dst_directory)) Done....I think the code you suggested is better thanks!

http://bugs.python.org/review/16278/