Skip to content

stubtest: rewrite #8325

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 74 commits into from
Feb 6, 2020
Merged

stubtest: rewrite #8325

merged 74 commits into from
Feb 6, 2020

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Jan 24, 2020

Hello,

I recently embarked on a rewrite of stubtest with the goal of improving our testing of typeshed. For some more context, please refer to python/typeshed#754 That issue also links examples of output and of typeshed issues this has detected.

Some notes about what this change contains:

  • Previously stubtest.py used dumpmodule.py to generate a JSON representation of runtime objects. This abandons that approach in favour of directly using runtime objects, allowing us easier and more dynamic introspection.
  • Much of stubtest.py was unimplemented, most notably, examining functions. This provides new and fairly detailed implementations of verify_funcitem, verify_overloadedfuncdef, verify_var, etc.
  • In particular, we can now do everything (and more!) @JukkaL mentioned in the original post at Better testing of stubs typeshed#754
  • There's a more complete CLI, with features to make testing typeshed easier.
  • There's more documentation.

My goal with this draft PR is to solicit code review and feedback from folks familiar with mypy internals. Some notes:

  • I'm still working on it!
  • The code doesn't match mypy's style, eg, line length is shorter than 99 and the script currently doesn't support py35
  • I haven't written any tests yet; I've just been running this against typeshed.
  • There's some discussion about where this script should live over at Better testing of stubs typeshed#754 -- curious if mypy has opinions on this.
  • The previous stubtest.py has been broken by changes to mypy internals multiple times before. If we can merge it, is the extra maintenance something mypy is okay with? We'd also have to come up with good CI tests that aren't checking typeshed, to avoid horrible cross-project build dependencies.

Anyway, let me know your thoughts!

Resolves #3329

hauntsaninja added 30 commits January 16, 2020 09:53
This will cause us to assume python version is sys.version_info, which
is the behaviour we want, since we're comparing things to the runtime
Rework things to avoid false positives / order nitpicks
Make checks involving *args, **kwargs a little more sophisticated
@hauntsaninja hauntsaninja marked this pull request as ready for review January 28, 2020 23:12
hauntsaninja added 2 commits January 28, 2020 16:06
is_subtype would always return False, leading to false positives most
times TypeVars were used for parameters with default values. We still
have some false positives from Unions of TypeVars, but that's less bad,
and could almost all be fixed by adjusting the overload of Mapping.get
@hauntsaninja
Copy link
Collaborator Author

I'm done making changes; this is ready for review 😄

@hauntsaninja
Copy link
Collaborator Author

Okay, I lied. But only because I didn't expect to run into a Python bug. (Up till now I'd just been testing on my Mac, bug is Linux-only because it manifests with select.epoll)

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Jan 31, 2020

(This comment contains a patch that detects issues with base classes. Unfortunately, in practice it's noisy; I've contributed the valuable ones I spotted to typeshed. Sticking this here, in case I need to find it again)

--- a/scripts/stubtest.py
+++ b/scripts/stubtest.py
@@ -213,6 +213,31 @@ def verify_mypyfile(
         )
 
 
+def _verify_base_classes(
+    stub: nodes.TypeInfo, runtime: Type[Any], object_path: List[str]
+) -> Iterator[Error]:
+    stub_bases = [(t.type.module_name, t.type.name) for t in stub.bases]
+    runtime_bases = [(t.__module__, t.__name__) for t in runtime.__bases__]
+
+    def should_filter(module_name: str, class_name: str) -> bool:
+        return (
+            module_name in ("typing", "collections.abc", "abc", "contextlib")
+            or (module_name, class_name) == ("builtins", "object")
+        )
+
+    filtered_stub_bases = [t[1] for t in stub_bases if not should_filter(t[0], t[1])]
+    filtered_runtime_bases = [t[1] for t in runtime_bases if not should_filter(t[0], t[1])]
+    if filtered_stub_bases != filtered_runtime_bases:
+        yield Error(
+            object_path,
+            "is inconsistent, base classes differ.",
+            stub,
+            runtime,
+            stub_desc=repr(stub) + ": " + ", ".join(".".join(t) for t in stub_bases),
+            runtime_desc=str(runtime) + ": " + ", ".join(".".join(t) for t in runtime_bases)
+        )
+
+
 @verify.register(nodes.TypeInfo)
 def verify_typeinfo(
     stub: nodes.TypeInfo, runtime: MaybeMissing[Type[Any]], object_path: List[str]
@@ -224,6 +249,8 @@ def verify_typeinfo(
         yield Error(object_path, "is not a type", stub, runtime, stub_desc=repr(stub))
         return
 
+    yield from _verify_base_classes(stub, runtime, object_path)
+
     to_check = set(stub.names)
     to_check.update(m for m in vars(runtime) if not m.startswith("_"))

@hauntsaninja
Copy link
Collaborator Author

Oops, didn't notice that one of those broke CI

@JelleZijlstra
Copy link
Member

Before I merge this, I'd like to make sure that there's something in CI that makes sure stubtest doesn't get broken by changes in other areas of mypy. From https://travis-ci.org/python/mypy/jobs/641260424?utm_medium=notification&utm_source=github_status it looks like the mypy self-check already covers stubtest, which is good.

Would it be possible to add a test that invokes stubtest on either the typeshed submodule or on some special test file? The latter would have the advantage that you could also test for error cases, but the former might be easier to set up.

@hauntsaninja
Copy link
Collaborator Author

I can do that. Currently we don't package stubtest, so tox knows nothing about it. I think it would make sense to treat this like stubgen, so move it into mypy/ and add a console script entry_point in setup.py? (I'd also prefer to do it in a separate diff, but can keep stacking on to this too)

@JelleZijlstra
Copy link
Member

Sure, that sounds good. I'll merge this PR now so that you can do the testing in a followup PR.

@JelleZijlstra JelleZijlstra merged commit 17b0a7a into python:master Feb 6, 2020
@hauntsaninja hauntsaninja deleted the stubtest2 branch February 6, 2020 06:42
@hauntsaninja hauntsaninja mentioned this pull request Feb 7, 2020
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.

Document and improve stubtest
3 participants