Repository navigation
Conversation
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Bolotnyj
changed the title
Bolotnyj
changed the title
Bolotnyj
changed the title
Bolotnyj
changed the title
Bolotnyj
changed the title
Bolotnyj
changed the title
|
Please don't just change the issue title. Simply wait for a maintainer to add corresponding labels if necessary. |
| @@ -0,0 +1 @@ | |||
| Added new unit tests to improve test coverage for the filecmp module. | |||
There was a problem hiding this comment.
There is no need for that NEWS entry.
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
- Don't put the tests just after the setUp() method, it doesn't make sense. Please be more mindful of existing code.
- What are we exactly testing and for which reason exactly as well? phase2() looks like a private (despite its public name) method which doesn't make sense to test specifically (it's not documented in https://docs.python.org/3/library/filecmp.html).
- Are we simply testing incomplete findings that only occur in phase2()? we usually don't test implementation details like that though.
- A lot of your tests seem to share a common setup. Why not extending setUp() or simply create a new class just for testing phase2?
| if os.path.exists(d_path): | ||
| os.rmdir(d_path) | ||
|
|
||
| def test_dircmp_invalid_attribute_error(self): |
There was a problem hiding this comment.
Why should we add this test? it's a useless test IMO.
| try: | ||
| with patch("os.stat", side_effect=fake_stat): | ||
| dc = filecmp.dircmp(self.dir, self.dir_same) | ||
| dc.phase2() | ||
| self.assertIn(trigger_file, dc.common_funny) | ||
| finally: | ||
| for d in (self.dir, self.dir_same): | ||
| p = os.path.join(d, trigger_file) | ||
| if os.path.exists(p): | ||
| os.remove(p) |
There was a problem hiding this comment.
Use self.addCleanup and a shutil.rmtree call.
|
|
||
| self.assertIn(file_diff_type, dc.common_funny) | ||
| self.assertIn(file_funny_type, dc.common_funny) | ||
| finally: |
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
picnixz
changed the title
picnixz
changed the title
filecmp
|
Picnixz, I have taken your words into account and will correct the errors. |
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @picnixz: please review the changes made to this pull request. |
2 participants
Added new unit tests to improve test coverage for the
filecmpmodule.Changes:
test_cache_overflow_and_hittoFileCompareTestCase.DirCmpImplementationTeststo testdircmpinternals through the public API (without relying on implementation details likephase2orphase4_closure).test_dircmp_os_errors_in_processingto verifyOSErrorhandling.test_dircmp_different_types_and_funny_typesto check type handling behavior.test_dircmp_closure_and_report_funniesto verify recursive closure processing and report output.try/finallyblocks with standardself.addCleanup()calls for reliable resource cleanups.