-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Unify test suites' module exclusion logic #14575
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@hauntsaninja wdyt? |
also @ilevkivskyi |
This comment has been minimized.
This comment has been minimized.
@JukkaL do you agree with the general approach that we should have fewer one-offs on the test suite level and a more consistent approach about what's included in the asserted-on results? |
# semantic analyzer. | ||
self.processed_targets: list[str] = [] | ||
# semantic analyzer. Tuple of module and target name. | ||
self.processed_targets: list[tuple[str, str]] = [] |
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.
processed_targets
is (as the comment says) for testing only, specifically for TypeCheckSuite
.
I'm adding the module's name so that TypeCheckSuite
could perform filtering more coherently:
actual = [
- t
- for t in actual
- if not any(t.startswith(mod) for mod in core_modules + ["mypy_extensions"])
+ target
+ for module, target in res.manager.processed_targets
+ if module in testcase.test_modules
]
@@ -41,6 +41,14 @@ class DeleteFile(NamedTuple): | |||
FileOperation: _TypeAlias = Union[UpdateFile, DeleteFile] | |||
|
|||
|
|||
def _file_arg_to_module(filename: str) -> str: |
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.
Not sure if "there's a function for that" in stdlib, but seemed easy enough to add one here. (I'm sure it misses some cases, just wanted something short and practical.)
@JukkaL 👋 |
This comment has been minimized.
This comment has been minimized.
@JukkaL could you take a look please? |
This comment has been minimized.
This comment has been minimized.
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
I did a quick pass and the overall approach looks good. I don't have time for a careful review right now, which this requires since there could be some risk of test false negatives. Does anybody have bandwidth to review this PR? |
This PR mostly changes how the "out" data is produced. It's not touching the assertion/comparison logic. Assuming this claim is correct (please review!), I'm not seeing a great risk of test false negatives (agreed?). |
@ilevkivskyi @hauntsaninja would you have bandwidth to review? As I said, in terms of |
Individual test suites grew to have distinct approaches for selecting or excluding modules / symbols, with distinct ad-hoc exclude lists. Rather than rely on a common set of rules, test authors have been adding to those distinct lists (having to modify the test suite to support their test case).
In this PR, we will consolidate this logic so we could have a common set of rules:
__main__
moduletested_modules
: modules provided through[file ...]
,[outfile ...]
or[outfile-re ...]
[builtins ...]
or[typing ...]
will not be part of the "tested corpus".[file ...]
to also only have a supporting role and not be part of the tested corpus. In tests we used to have conventions like excluding modules starting with_
. Instead, we'd have an explicit[fixture ...]
block that just like[file ...]
except it doesn't participate in the "tested corpus".