Skip to content
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

Merged
merged 10 commits into from
Mar 2, 2023

Conversation

ikonst
Copy link
Contributor

@ikonst ikonst commented Feb 1, 2023

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:

  • The "tested corpus" is what's asserted on. Everything else has a supporting role but does not contribute to what's being asserted on.
  • The "tested corpus" is
    • the __main__ module
    • the tested_modules: modules provided through [file ...], [outfile ...] or [outfile-re ...]
  • It follows that library code, whether imported from lib-stub/ or provided through [builtins ...] or [typing ...] will not be part of the "tested corpus".
  • At times we want [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".

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@ikonst ikonst changed the title Reduce DataSuite use of module exclusion lists Unify test suites' module exclusion logic Feb 2, 2023
@github-actions

This comment has been minimized.

@ikonst
Copy link
Contributor Author

ikonst commented Feb 2, 2023

@hauntsaninja wdyt?

@ikonst
Copy link
Contributor Author

ikonst commented Feb 3, 2023

also @ilevkivskyi

@github-actions

This comment has been minimized.

@ilevkivskyi
Copy link
Member

@ikonst I think this is rather where @JukkaL may have some opinions.

@ikonst
Copy link
Contributor Author

ikonst commented Feb 8, 2023

@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]] = []
Copy link
Contributor Author

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:
Copy link
Contributor Author

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.)

@ikonst
Copy link
Contributor Author

ikonst commented Feb 14, 2023

@JukkaL 👋

@github-actions

This comment has been minimized.

@ikonst
Copy link
Contributor Author

ikonst commented Feb 19, 2023

@JukkaL could you take a look please?

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JukkaL
Copy link
Collaborator

JukkaL commented Feb 24, 2023

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?

@ikonst
Copy link
Contributor Author

ikonst commented Feb 24, 2023

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?).

@ikonst
Copy link
Contributor Author

ikonst commented Mar 2, 2023

@ilevkivskyi @hauntsaninja would you have bandwidth to review?

As I said, in terms of assert actual == expected, it affects how the actual is made, not how == works. So as long as we're happy with how it picks what to include in the output (you can see it from the tests I had to change), then it should be alright.

@ilevkivskyi ilevkivskyi merged commit 099500e into python:master Mar 2, 2023
@ikonst ikonst deleted the 2023-02-01-test_paths branch March 2, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants