The filecmp module has no tests for the following methods of the dircmp class:

report()
report_partial_closure()
report_full_closure()

The attached patch provides those tests. This issue was suggested in issue 15454. Future plans for these tests are provided in that issue.

Thanks, Chris.  A couple high-level comments.

I know this isn't the prevailing style of the existing tests, but it's generally better if each unit test tests just one thing.  That way, for example, if a test fails you know more precisely what is not working.

Would it be possible to structure things so that you have tests like test_report(), test_report_partial_closure(), etc?

Also, is there any reason you need to be using a regular expression as opposed to an exact string match?

> Would it be possible to structure things so that you have tests like test_report(), test_report_partial_closure(), etc?

Yes, I would prefer that structure. I was just being consistent, as you said, with the prevailing style of the existing tests. I will provide another patch with tests separated into report method concerns as you suggest and I agree.

> Also, is there any reason you need to be using a regular expression as opposed to an exact string match?

Yes, the setUp method uses tempfile-generated directory names (and that's actually one of the few good things about the existing tests). The tempfile-generated directory names are part of the report method output and are not known in advance. So we cannot construct an exact string match in advance.

> The tempfile-generated directory names are part of the report method output and are not known in advance.

There are at least two ways you could deal with this.  You could change the working directory to the temp directory in setUp() and change it back in tearDown().  This is a common pattern in the tests, and there is even a temp_cwd() context manager in Lib/test/support.py that will do this for you automatically (which unfortunately you can't use as is until issue 15351 is addressed).  This way the report output would be relative to the temp directory, and so you wouldn't need to know the absolute path.  To me this approach is preferred because the report output is more readable (no long file paths, etc).

Alternatively, you could save the temp directory path as an attribute of the test class, and then include it in your string match prior to calling the assert method.

> There are at least two ways you could deal with this.  You could change the working directory to the temp directory in setUp() and change it back in tearDown().  This is a common pattern in the tests [...]

> Alternatively, you could save the temp directory path as an attribute of the test class, and then include it in your string match prior to calling the assert method.

Although I prefer explicitly testing that the subdirectory paths shows up in the reports and, in the case of the report_partial_closure test, the regex explicitly tests that the "only in" subdirectory is reported correctly, if I were to have to choose between the two options you propose, I would choose decorating the test instance with the strings as that would at least preserve the ability to perform the latter of the two explicit tests I mention and would not be subject to yet another file system point of failure while switching directories (even if that is a common pattern in the tests).

assertRegex is new from unittest2 and seems tailor-made for this use case to me. It gets the job done in explicitly correctly in the least amount of code. If you insist that I not use assertRegex for the report test, I will yield to your preferences. However, I would like your blessing to continue with assertRegex in this case.

In either event, I still will refactor the patch into separate test methods for the report methods as previously agreed after your continued feedback. And thank you again for your feedback above.

> However, I would like your blessing to continue with assertRegex in this case.

Sure, you can proceed that way. :)  My suggestions were just that -- suggestions for you to consider.  And I am not a core developer, so my approval won't necessarily get your patch committed more quickly. :)  Thanks for considering my suggestions.

Well, really, the reason I'm deferring is to get the patch accepted, because it seems kind of unacceptable for standard library modules not to have full test coverage. So far, I don't see evidence that the issue has even been triaged. Of the three approaches, regex, cd, or decoration, which do you see as most likely to be accepted? I understand that cd might be common in some tests. But it also appears that, at least in test_filecmp, some pretty heinous practices are also common like piling a ridiculous number of asserts into one test method. (And I'll fix that, too.) I do really appreciate your advice, core developer or not. You are the only one providing advice here.

> Of the three approaches, regex, cd, or decoration, which do you see as most likely to be accepted?

In my limited experience, I think the patch most likely to be accepted is the one that changes existing code the least. That would mean adding to the practices you're observing, unfortunately. Personally, I would go with trying to use test.support.temp_cwd() and doing an exact string match (since it provides a more precise test) -- taking appropriate cross-platform measures as necessary.

> I do really appreciate your advice, core developer or not. You are the only one providing advice here.

You're welcome. I'm in the same boat as you, so I know how it feels.

Moving to 3.4

In 3.3 we won't add functionality to filecmp, but in 3.4 we will, so the new tests should go there.

Chris Calloway, let's move this forward. I want to see more tests for filecmp ASAP.

Please address Chris Jerdonek's points in a new patch made vs. fresh default branch (3.4), at this point "cd"ing (before the test_cwd thing is done). For testing code, practicality beats purity. Having these tests in place will help moving other issues forward (i.e. #15454). Also, as Chris Jerdonek has mentioned, having as little as possible in each test_ method is a good practice, and older test code that doesn't follow it can be fixed later (patches welcome).

I have a number of patches for this issue I have worked on during a few local Python user group hack nights. I have not submitted them yet, however, as they require some expository material here in order to illustrate how they address Jerdonek's points. I have a regional Python conference (PyCarolinas) that I have some responsibilities in through the weekend, however, and will not be able to move forward ASAP until after then.

Chris Calloway, would you like to pursue this issue further?

Yes, that's why I opened it. Feel free to submit patches if you think I'm moving too slowly.

Chris, your reply seems to indicate that you're irritated for some reason, and I'm not sure why.

I was simply going through my open issue backlog, and since this is something I'd like to see fixed, I noticed that you said in October that you have some patches so I pinged you as a reminder. This is pretty accepted practice in the issue tracker. It doesn't imply that "you're moving too slowly", since this isn't really a critical issue.

OK, I got to participate in my Python user group hack night this week where I actually got to work on my projects instead of spending all night helping newbies on their projects. I used the time to refactor all the previous patches for this issue on which I had previously been working into the attached issue-15518-1.patch (replacing previous patch of the same name). I believe this patch a) provides the necessary minimum test coverage for the report methods, b) addresses chris.jerdonek's and eli.bendersky concerns, and c) does a strict PEP8 clean-up of the pre-existing tests (without changing pre-existing code). If you like this patch, I'd be glad to refactor the pre-existing tests to a) put each pre-existing test in its own TestCase methods and b) clean-up the unmaintainable proliferation of repeated hard-coded strings in the pre-existing tests. Feedback appreciated.

Replaced patch to fix a few under-informative test messages.

A lot of the code in the tests is devoted to building the tested directory tree and populating it ( _setUpDirectories and related functions). You keep building and deleting this tree for every running test.

I think that a better idea would be to just create this tree somewhere in Lib/test (look at the directories under it - we can add a new one, like filecmpdata/) and then refer to it in the tests. This will remove a huge amount of code and should also make it easier to understand what exactly the contents of this tree are. For example then I can use other system Linux tools to explore the directory and figure out what I'd expect the tests to do.

When test data is very small and contained, it makes sense to just create it while the test is running. But the patch has > 120 LOC for that with functions calling other functions. For someone not intimately familiar with the tests, it's hard to follow which files get created and their contents.

Other comments:

* Don't use external functions with 'self' to serve essentially as methods. Test classes can derive from some common class that provides such functionality. [not sure this will be relevant after the above refactoring]
* If you do pretty much the same thing for every tearDown, also consider refactoring it into a base class.

The attached file test_filecmp_layouts.rst documents the directory and file layouts for complete test coverage of filecmp.dircmp report methods to aid in review discussion of patch.

The attached file test_filecmp_reports.rst documents the reports generated by filecmp.dircmp test methods if the directory and file layouts in test_filecmp_layouts.rst are followed to aid in review discussion of patch.

The attached patch issue-15518-1.patch replaces previous patch proposals for this issue.

The patch implements the suggestion to factor code common to report TestCase classes into a common base class BaseReportTestCase. Apologies for not having that in the previous patch.

Concerning the major point of msg186123, I had anticipated that there would need to be some discussion concerning the complexity of test data layouts and expected reports. So I have attached two visual aids to facilitate review and feedback. I made many such graphs when tracing the minimal path for complete test coverage and found them useful.

test_filecmp_layouts.rst details the directory and file layouts minimally necessary to provide complete test coverage for the report methods in filecmp.dircmp. The layouts are shown in pretty-printed os.walk format. test_filecmp_reports.rst details the reports which result from those layouts when all filecmp.dircmp report methods are exercised against them. The reports are shown as rendered by printing.

Both visual aids are laid out as tables, with TestCase names as column headers, and test_ method names within those TestCases as row headers. Both visual aids identify identical cells by an index number in the bottom right hand corner of each cell for cells that are duplicated.

The layouts table identifies six common directory and file layouts that would be necessary within a proposed filecmpdata directory. I had created these layouts in setUp methods in order to follow the pre-existing style of the test script. I agree that doing so added to the test code complexity. I agree with the suggestion to instead put the layouts in a filecmpdata directory. Before I revise the patch to do that, however, I just want in understood that there are six layouts minimally necessary in the proposed filecmpdata.

Placing the layouts in a filecmpdata directory will eliminate the need for a tearDown method in BaseReportTestCase. Placing layouts in a filecmpdata directory will faciliate the revision of the pre-existing TestCases in the script, as they use similar layouts currently created in setUp methods and modified in test_ methods.

Fix calls to BaseReportTestCase methods in subclasses. Not an override so don't need super().

I replaced issue-15518-1.patch. I believe it now addresses all the concerns expressed thus far. If reviewed favorably, I will extend it to clean-up the pre-existing tests to address the concerns noted about them, especially since the filecmpdata directory added by this patch contains the information necessary to test the rest of the filecmp module rather than resorting to creating temp directories and files. It seems less than ideal to me to have the test script use a mixed model for accessing test data (both temp and filecmpdata directories). But that's not my call.

Thanks, looks much better now. I left a couple of small comments in the code review. Other than that, it makes sense to me to update the existing tests to follow this scheme as well. Thanks!

On a related note, since you've obviously studied the filecmp module's operation to a great depth, your input on http://bugs.python.org/issue15430 would be appreciated.

I'm uploading a new patch which gets rid of the temp_cwd calls as suggested in the review. The patch is not complete in that the explanatory comments suggested in the review and revising the pre-existing tests have not been completed yet. However, I want to upload the work I do have as it has been a couple of weeks since reivew and I won't have another opportunity to complete the review suggestions until PUG project nights on May 7th and 8th.

Also, I'm uploading this new patch because the previous patch had a serious problem that caused this delay. I happened to get a new laptop and used the previous patch to recreate the tests. I found that part of the new filecmpdata directory was missing when I did that and some of the tests failed as a result.

It turns out that hg add and hg diff were not detecting all of filecmpdata. There's a well known problem with hg no detecting empty directories. However, this was not the case in this patch. Because I was able to workaround the problem by recreating all the dir-same directories and all their subdirectories, I think the problem is with hg not detecting directories with identical contents if the creation dates are the same. It's something I'll need to investigate more when this issue has been completed.

Thanks for the heads up Chris. Since I'm a bit busy myself lately, it's fine to wait until you have time to complete the patch. I'll review it then.

The promised comments have been added to the patch. The refactoring of the pre-existing tests is not part of this patch. But I'm uploading this now as the patch does fix the issue.

Reformat the filecmpdata directory layouts diagram.

Reformat the filecmp test report matrix.

Formatted test directory layout.

Format the martix of test reports.