-
-
Notifications
You must be signed in to change notification settings - Fork 959
Fix bugs affecting exception wrapping in rmtree callback #1700
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
2814263
Add a missing PermissionError xfail on Windows
EliahKagan fba59aa
Update "ACTUALLY skipped by" comments
EliahKagan 5039df3
Eliminate duplicate rmtree try-except logic
EliahKagan 683a3ee
Clean up git.objects.submodule.base imports
EliahKagan 2fe7f3c
Test current expected behavior of git.util.rmtree
EliahKagan d42cd72
Test situations git.util.rmtree shouldn't wrap
EliahKagan 2a32e25
Fix test bug that assumed staticmethod callability
EliahKagan b8e009e
In rmtree, have onerror catch only PermissionError
EliahKagan ccbb273
Fix onerror callback type hinting, improve style
EliahKagan 0b88012
Use onexc callback where supported
EliahKagan 7dd5904
Revise and update rmtree docstrings and comments
EliahKagan 196cfbe
Clean up test_util, reorganizing for readability
EliahKagan 100ab98
Add initial test_env_vars_for_windows_tests
EliahKagan 7604da1
Warn if HIDE_WINDOWS_*_ERRORS set in environment
EliahKagan eb51277
Make HIDE_* attributes always bool
EliahKagan 333896b
Treat false-seeming HIDE_* env var values as false
EliahKagan c11b366
Simplify HIDE_* env var test; add missing cases
EliahKagan f0e15e8
Further cleanup in test_util (on new tests)
EliahKagan a9b05ec
Clarify a test helper docstring
EliahKagan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Add initial test_env_vars_for_windows_tests
The new test method just verifies the current behavior of the HIDE_WINDOWS_KNOWN_ERRORS and HIDE_WINDOWS_FREEZE_ERRORS environment variables. This is so there is a test to modify when changing that behavior. The purpose of these tests is *not* to claim that the behavior of either variable is something code that uses GitPython can (or has ever been able to) rely on. This also adds pytest-subtests as a dependency, so multiple failures from the subtests can be seen in the same test run.
- Loading branch information
commit 100ab989fcba0b1d1bd89b5b4b41ea5014da3d82
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,4 +7,5 @@ pre-commit | |
| pytest | ||
| pytest-cov | ||
| pytest-instafail | ||
| pytest-subtests | ||
| pytest-sugar | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used the
subTestmethod ofunittest.TestCaseintest_env_vars_for_windows_tests. Thispytestplugin givespytestfull support for that mechanism. It letspytestcontinue with the other subtests even after one fails, as theunittesttest runner would, and show separate passes and failures for the individual subtests. Separate results are shown only when at least one fails, so when everything passes the report remains uncluttered. (It also provides asubtestspytest fixture, though since this is not an autouse fixture it is not usable from within a method of a class that inherits fromunittest.TestCase.)I think this is useful to have going forward, since we have many test cases that are large with many separate assertions of separate facts about the system under test, and as they are updated, some of them could be improved by having their separate claims divided into subtests so they can be individually described and so failures don't unnecessarily block later subtests.
However, if you'd rather this plugin be used, it can be removed.
test_env_vars_for_windows_testscould retain subtests--they will still each run if none fails, just like multiple assertions in a test case without subtests. Or I could replace the subtests with more@ddtparameterization, or manually, etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I all sounds reasonable and in particular, since you are the one doing the work it seems fair that you can use the tooling you see as best fit. I also have no experience here and no preferences, and believe that anything that improves the tests in any way is very welcome. Thank you!
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. For the conceptually unrelated reason that it would facilitate more fine-grained
xfailmarking--so we don't habitually carry XPASSing cases that aren't actually expected to have failed--I think the test class involved here,test.test_util.TestUtils, should be split, so that thecygpath-related tests can be purepytesttests. The reason to split it is:@pytest.mark.parametrizesupports fine-grained marking of individual test cases, including asskiporxfail, but that form of parameterization cannot be applied to test methods inunittest.TestCasesubclasses. So at least those tests would currently (i.e., until the underlying causes of some cases failing are addressed) benefit from being purepytesttests.cygpath, use therorepofixture provided bytest.lib.helper.TestBase, which inherits fromunittest.TestCase. Although this could be converted to apytestfixture, I'd rather wait to do that until after more operating systems, at least Windows, are tested on CI, and also until I have more insight into whether it makes sense to do that at all, rather than replacingrorepoand other fixtures with new corresponding fixtures that use isolated repositories (#914, #1693 (review)). So at least those tests should currently remain in aunittest.TestCasesubclass.So long as it's acceptable to have multiple test classes in the same test module, this could be done at any time, and it may facilitate some other simplifications. I mention it here because I think it might lead to the elimination of subtests in this particular module, either by using
@pytest.mark.parametrizefor this too or for other reasons.If that happens, I might remove the
pytest-subteststest dependency, even though it might be re-added later, because the alternative ofpytest-checkmay be preferable in some of the large test methods if they can also be converted to be purepytesttests (becausepytest-checksupports a more compact syntax).