Thanks for the review...I think the patch is getting better and better. Soon I should have this tested on Windows, hopefully tonight. http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py File Lib/test/test_os.py (right): http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode220... Lib/test/test_os.py:2209: class RenameTests(unittest.TestCase): On 2013/03/11 00:45:35, vstepanov wrote:

I have some comments to this patch. I think would be better to add two separate test case RenameFileTests and RenameDirTests or something like that. I actually like it in a single class because alot of the functionality is the same for testing the rename of a directory or file. If we separate it into two classes then I am afraid that we get lots of repeating which leads to DRY test cases. See earlier comments in issue.

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode221... Lib/test/test_os.py:2215: self.rename_src_file = "%s_%d_tmp" % ("test_rename_src_file", On 2013/03/11 00:45:35, vstepanov wrote:

All variables has prefix ``rename_`` (rename_src_file, rename_dst_file and so on), so can use them without this prefix (just src_file, dst_file ...) Good point....this is a side effect from me moving over to a new class before it was in the FileTests directory. Now that we have a RenameTests class we can remove the rename_ in front of all the variables this will help readability. Thanks!

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode221... Lib/test/test_os.py:2216: os.getpid()) On 2013/03/11 00:45:35, vstepanov wrote:

should not call this method everywhere, use a variable Yes that will be more efficient, thanks!

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode221... Lib/test/test_os.py:2217: if os.path.exists(self.rename_src_file): On 2013/03/11 00:45:35, vstepanov wrote:

There is remove_file_or_directory method, use it for remove exist files/dirs. Good point, I don't know how I missed the remove_file_or_directory in setUp but I did. Thanks.

Really need to write many lines in a file and then split the text on

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode222... Lib/test/test_os.py:2220: os.write(fd, b"beans\n") On 2013/03/11 00:45:35, vstepanov wrote: the line in

tests? Perhaps enough only one. I got the original code from test_os.py line 111 in the TestWrite method. But it does not have to be that way I think the TestWrite method was testing writes with binary data, byte array data, and memory view data. This is actually easier to read and doing the split lines doesn't add any value. Thanks.

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode227... Lib/test/test_os.py:2277: if (os.path.isfile(file)): On 2013/03/11 00:45:35, vstepanov wrote:

outer parentheses are unnecessary Yes too much scheme sorry I missed those, they are gone now.

I do not see where `src` and `dst` variables are declared Oops I didn't catch that because I have not tested on Windows yet. I do

I think it would be better to create the files/directories in separate methods and call them in tests methods instead of create them in setUp method and remove in each test (except for those that are not removed in tests methods,

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode228... Lib/test/test_os.py:2289: self.assertRaises(OSError, os.rename, src, dst) On 2013/03/11 00:45:35, vstepanov wrote: plan to test on Windows soon maybe even tonight. My Windows partition is not setup correctly yet since I got my new computer back in October. http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode229... Lib/test/test_os.py:2295: self.remove_file_or_directory(self.rename_dst_file) On 2013/03/11 00:45:35, vstepanov wrote: they can

be left in the setUp). I have been wondering if there is a better approach to all of this. According to some of the CPython Committers we have to make the test case as WET as possible and not repeat ourselves. Before I was actually creating the files/directories in the test cases but was not using a method it took a lot of code. I could make a version that does that but use methods to create the files instead then measure the line count to see which solution is WETTEST. Let me think about this....

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode232... Lib/test/test_os.py:2326: self.assertEqual(os.path.exists(self.rename_dst_directory), True) On 2013/03/11 00:45:35, vstepanov wrote:

There are unittest.TestCase.assertTrue and unittest.TestCase.assertFalse methods. Yes I fixed some of them with V2 but V3 now has all of them fixed. Thanks.

http://bugs.python.org/review/16278/diff/7565/Lib/test/test_os.py#newcode240... Lib/test/test_os.py:2402: src = self.rename_src_file + "does_not_exist" On 2013/03/11 00:45:35, vstepanov wrote:

I don't see where `src` and `dst` are used. You are correct I don't need those thanks!

I will post a new version soon.... http://bugs.python.org/review/16278/