Refactor testpep561 and add test to address #5767 #5782

chrisphilip322 · 2018-10-12T20:57:03Z

I took some liberties while refactoring this to more fit my personal style and hopefully make more readable/extensible. I also added test cases for all the edge cases I found and filed in #5758; including a test case for which #5767 was filed.

I understand this is a pretty big change to this file and I am open to redoing it if its not popular.

gvanrossum · 2018-10-12T22:44:12Z

Let's make sure this catches #5784

gvanrossum · 2018-10-12T22:52:08Z

I would very much like to use these tests to prove that I've fixed #5784 (rather than just manually running the example shown there). But for that to happen this would probably have to be brought back to minimally invasive form -- or at least it would have to pass the tests on AppVeyor.

chrisphilip322 · 2018-10-12T23:29:56Z

Woops sorry, I'm moving too fast. I just saw I got an error and commited it. I'll work on actually repro'ing now that I've properly read the issue.

chrisphilip322 · 2018-10-13T01:09:19Z

Build failures so far are because of #5784. Right now, I am not really sure what the test should actually look like but for now it reproduces the issue at least, I will make a better test once the issue gets fixed.

gvanrossum · 2018-10-13T01:59:44Z

See if the one-line fix in #5785 (latest version) fixes it.

…nce-testpep561

chrisphilip322 · 2018-10-13T02:15:02Z

#5785 worked locally

ethanhs

I won't claim to fully understand the issue in the 0.640 release yet (I will finally have time to look at it later today) but I don't think the C-extension complexity is needed. While in the original report it was an issue with the C-extension in asyncio, we also got a report with a normal Python module. Since this adds a fair amount of complexity, perhaps we could do something a bit simpler?

chrisphilip322 · 2018-10-14T00:47:16Z

@ethanhs could you share a link to the repro that doesn't use c extensions?

gvanrossum · 2018-10-14T00:53:12Z

I've got a hunch that what you need to repro the issue is an installed package with a py.typed marker, and an import of a non-existent submodule of that package with a # type: ignore on the import. See the repro in #5784 -- since mypy doesn't see the C extension it doesn't matter whether it's installed or not.

I think that you have to install at least something like pkg/mod.py and inside that do from pkg.other import blah # type: ignore.

gvanrossum

Some style comments, and a suggestion on how to avoid having to compile C code (which will make the tests fail on machines where the user hasn't installed a C compiler -- these days all too common).

On my machine (update: a Mac) test_typedpkg_editable() fails consistently, as follows:

Traceback (most recent call last):
  File "/Users/guido/src/mypy/mypy/test/testpep561.py", line 280, in test_typedpkg_editable
    venv_dir=venv_dir,
  File "/Users/guido/src/mypy/mypy/test/testpep561.py", line 123, in check_mypy_run
    assert out == self.build_msg(*expected_out), err
AssertionError: 
assert "/var/folders...pe is 'Any'\n" == "/var/folders/...ltins.str]'\n"
  - /var/folders/63/czkyq6090dd0t157zhx54xvhrdlybt/T/tmpo6_mhk_f/test_program.py:2: error: Cannot find module named 'typedpkg.sample'
  - /var/folders/63/czkyq6090dd0t157zhx54xvhrdlybt/T/tmpo6_mhk_f/test_program.py:2: note: (Perhaps setting MYPYPATH or using the "--ignore-missing-imports" flag would help)
  - /var/folders/63/czkyq6090dd0t157zhx54xvhrdlybt/T/tmpo6_mhk_f/test_program.py:3: error: Cannot find module named 'typedpkg'
  - /var/folders/63/czkyq6090dd0t157zhx54xvhrdlybt/T/tmpo6_mhk_f/test_program.py:5: error: Revealed type is 'Any'
  ?                                                                                                          ^ ^
  + /var/folders/63/czkyq6090dd0t157zhx54xvhrdlybt/T/tmpo6_mhk_f/test_program.py:5: error: Revealed type is 'builtins.tuple[builtins.str]'
  ?                                                                                                          ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^

(Also, these tests still take way too long to run. But I understand that running pip install is just slow...)

gvanrossum · 2018-10-14T03:20:48Z

mypy/test/testpep561.py

+
+class NamespaceProgramImportStyle(Enum):
+    from_import = """\
+from typedpkg_nested.nested_package.nested_module import nested_func


I dislike having those long module names. If you give each of them names of 3-5 letters it will become more readable (and you won't have to break long lines with \ below). Also a comment may be in order explaining why some things must go together on one line (IIUC it's so that the line numbers in the error messages are consistent?)

gvanrossum · 2018-10-14T03:23:56Z

mypy/test/testpep561.py

-        self.namespace_msg_int_bool = (
-            '{0}:9: error: Argument 1 to "alpha_func" has incompatible type "int"; '
-            'expected "bool"\n'.format(self.namespace_tempfile))
+        self.simple_example_program = ExampleProgram(SIMPLE_PROGRAM)


These variable names are all too long -- when skimming all I see is blah_blah_blah_blah. I don't think they all need to end in _example_program, and you can abbreviate namespace as ns.

gvanrossum · 2018-10-14T03:26:30Z

test-data/packages/typedpkg_c_ext/hello.c

+ ******************************************************************/
+#include <Python.h>
+
+// ot ForbesFunction 1: A simple 'hello world' function


gvanrossum · 2018-10-14T03:36:55Z

test-data/packages/typedpkg_c_ext/setup.py

+    name='typedpkg_c_ext',
+    version='1.0',
+    packages=['typedpkg_c_ext'],
+    ext_modules=[Extension('typedpkg_c_ext.hello', ['hello.c'])],


Looks like you can just delete this line (and the file hello.c) and the test works just as well. (Verified by removing the isdir() check from modulefinder.c line 124.)

gvanrossum · 2018-10-14T04:50:05Z

There's something else missing. I patched this in, and then put an early return in _update_ns_ancestors() in modulefinder.py. This should simulate not fixing part two of #5758, notably https://github.com/python/mypy/pull/5762/commits (with an insightful review comment that I ignored).

But all pep561 tests still pass with that change (except test_typedpkg_editable() still fails). So it looks like the requested test still doesn't exist...

…testpep561

chrisphilip322 · 2018-10-14T16:03:33Z

I reorganized the test files so there isn't as much overlap with the test packages and made names in general shorter. I removed the c extensions test package because the repro can be done without it; and I made sure that the #5767 case is covered.

Side Note: I am expecting the tests to fail because it looks like there is repeated warning output in some cases.

gvanrossum · 2018-10-14T17:09:06Z

But most tests are failing in the install phase -- is that also expected?

What warnings are you expecting exactly?

chrisphilip322 · 2018-10-14T17:38:36Z

I was reading the pytest output wrong, tests should pass now.

gvanrossum · 2018-10-14T21:36:20Z

I still get the failure on test_typedpkg_editable(). Any insight on how I should debug this?

ilevkivskyi · 2018-10-14T21:38:35Z

I still get the failure on test_typedpkg_editable(). Any insight on how I should debug this?

Are you under a virtualenv on a Mac? If yes, then this is not new, it fails since some time.

ilevkivskyi · 2018-10-14T21:43:14Z

I am seeing this:

E           AssertionError: 
E           assert "/var/folders...pe is 'Any'\n" == "/var/folders/...ltins.str]'\n"
E             Skipping 61 identical leading characters in diff, use -v to show
E             + simple.py:5: error: Revealed type is 'builtins.tuple[builtins.str]'
E             - simple.py:2: error: Cannot find module named 'typedpkg.sample'
E             - /var/folders/ff/8tg5kw8x3jg3z9cw0359hllxss39wp/T/tmph7q_wh1f/simple.py:2: note: (Perhaps setting MYPYPATH or using the "--ignore-missing-imports" flag would help)
E             - /var/folders/ff/8tg5kw8x3jg3z9cw0359hllxss39wp/T/tmph7q_wh1f/simple.py:3: error: Cannot find module named 'typedpkg'
E             - /var/folders/ff/8tg5kw8x3jg3z9cw0359hllxss39wp/T/tmph7q_wh1f/simple.py:5: error: Revealed type...
E             
E             ...Full output truncated (1 line hidden), use '-vv' to show

for few month, but I have never had time to fix it.

gvanrossum · 2018-10-14T21:48:48Z

Yeah, that's what I get too (the traceback I reported is about the same). And yes, I am under a venv.

ilevkivskyi · 2018-10-14T21:52:06Z

I think this is an issue with the test infrastructure, since the same scenario works fine with installed mypy. A natural guess is that something goes wrong because this test creates a new virtual environment that probably conflicts somehow with the current environment.

gvanrossum · 2018-10-14T22:28:51Z

Hm, I should really leave this alone, but I investigated a bit more and found that when an outer venv is already active, the line self.install_package('typedpkg', python_executable, editable=True) installs the typedpkg.egg-link file in the outer venv. And apparently the test cannot find it there. Possibly this is a pip issue -- or possibly the code in install_package() needs something extra to convince pip to use the inner venv.

gvanrossum · 2018-10-14T23:19:48Z

One more internal monologue and then I'll let it go. :-)

I added -v to the pip command and the crucial bits seem to be this:

Installing collected packages: typedpkg
  Found existing installation: typedpkg 0.1
    Not uninstalling typedpkg at /Users/guido/src/mypy/test-data/packages/typedpkg, outside environment /var/folders/63/czkyq6090dd0t157zhx54xvhrdlybt/T/tmp5ov0t1ad/bin/..
    Can't uninstall 'typedpkg'. No files were found to uninstall.

There's other verbose output before and after this, but in a successful run, instead of this I see:

Installing collected packages: typedpkg
  Found existing installation: typedpkg 0.1
    Uninstalling typedpkg-0.1:
      Created temporary directory: /private/var/folders/63/czkyq6090dd0t157zhx54xvhrdlybt/T/pip-uninstall-5hfyyz5b
      Removing file or directory /Users/guido/src/mypy/vv/lib/python3.7/site-packages/typedpkg.egg-link
      Removing pth entries from /Users/guido/src/mypy/vv/lib/python3.7/site-packages/easy-install.pth:
      Removing entry: /Users/guido/src/mypy/test-data/packages/typedpkg
      Successfully uninstalled typedpkg-0.1

Additional notes:

When I manually create two venvs (v1 and v2), activate v1, and run v2/bin/pip install -e .../typedpkg in the other, the egg-link file gets created in the right place.
If I don't activate v1 but run v1/bin/python3 -m pytest ... (so effectively v2 is created by the test program) the problem still appears.
I added another test, test_typedpkg_egg_editable(), which uses use_pip=False, editable=True (i.e. invoking setup.py develop directly) and that one passed.

gvanrossum

LGTM. If Ethan agrees this can be merged!

gvanrossum · 2018-10-15T00:30:30Z

mypy/test/testpep561.py

                python_executable,
-                expected_out=self.msg_dne + self.msg_list,
+                [SimpleMsg.msg_dne,
+                 SimpleMsg.msg_list],


(I don't care much for splitting this line -- the whole list easily fits.)

ethanhs

Two suggestions, but otherwise seems fine to me.
Thank you for working on this!

ethanhs · 2018-10-16T23:11:00Z

mypy/test/testpep561.py

+        self._temp_dir = None  # type: Optional[tempfile.TemporaryDirectory[str]]
+        self._full_fname = ''
+
+    def init(self) -> None:


Suggestion: rename to create? init doesn't really tell me what it is doing, create implies it is doing setup work and creating the test case.

ethanhs · 2018-10-16T23:11:40Z

mypy/test/testpep561.py

-        assert err == expected_err, out
-        assert returncode == expected_returncode, returncode
-    finally:
+class NSImportStyle(Enum):


I'm glad to see these going into Enums. :)

ethanhs · 2018-10-16T23:16:30Z

mypy/test/testpep561.py

+    return _NAMESPACE_PROGRAM.format(import_style=import_style.value)
+
+
+class ExampleProg(object):


Maybe change this to just Program and change the suffixes below to self.simple_ep to self.simple_prog or something? ep is not a straightforward mnemonic (at least to me).

…testpep561

gvanrossum

I'll merge this now. Thank you for this! (And thanks Ethan for the review.)

add testcase which closes python#5767

2b69726

chrisphilip322 force-pushed the enhance-testpep561 branch from 94c854d to 2b69726 Compare October 12, 2018 20:57

ethanhs self-requested a review October 12, 2018 21:06

Only return ns_ancestors in namespace-package mode

92e7a7d

Guido van Rossum and others added 3 commits October 12, 2018 15:55

Might as well use belt and suspenders

8bee0a3

use windows friendly tempfiles

7a33cb3

add test to repro python#5784

a0a4959

chrisphilip322 added 3 commits October 12, 2018 19:31

remove bad testcase

102cf3b

fix typing warning

9c577e6

add test to actually reproduce python#5784

146c26b

Better fix -- roll back the previous attempts in favor of isdir() check

6122776

chrisphilip322 added 2 commits October 12, 2018 22:12

enhance new test case

b02e6db

Merge branch 'fix-regression' of github.com:gvanrossum/mypy into enha…

7e1e3b2

…nce-testpep561

chrisphilip322 added 2 commits October 12, 2018 22:27

address flake8

fa6d6ab

add credits to the c extension test file

ad6952e

gvanrossum mentioned this pull request Oct 13, 2018

Avoid "No such file or directory" if module ancestor doesn't exist #5785

Merged

ethanhs requested changes Oct 14, 2018

View reviewed changes

gvanrossum reviewed Oct 14, 2018

View reviewed changes

chrisphilip322 added 3 commits October 14, 2018 11:40

update test to provide actual coverage

c166b16

Merge branch 'master' of https://github.com/python/mypy into enhance-…

f826d15

…testpep561

reorganize test files

f511005

chrisphilip322 added 2 commits October 14, 2018 13:18

add missing files

2cced84

edit expected messages

e7ab97c

gvanrossum approved these changes Oct 15, 2018

View reviewed changes

gvanrossum and others added 2 commits October 14, 2018 17:59

Add test_typedpkg_egg_editable() for completeness

7513cae

remove unneeded line breaks

73c3557

ethanhs approved these changes Oct 16, 2018

View reviewed changes

chrisphilip322 added 2 commits October 17, 2018 10:18

Merge branch 'master' of https://github.com/python/mypy into enhance-…

62e6f09

…testpep561

s/ep/prog and s/init/create

5826e0c

gvanrossum approved these changes Oct 17, 2018

View reviewed changes

gvanrossum merged commit c0f357c into python:master Oct 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor testpep561 and add test to address #5767 #5782

Refactor testpep561 and add test to address #5767 #5782

chrisphilip322 commented Oct 12, 2018

gvanrossum commented Oct 12, 2018

gvanrossum commented Oct 12, 2018

chrisphilip322 commented Oct 12, 2018

chrisphilip322 commented Oct 13, 2018

gvanrossum commented Oct 13, 2018

chrisphilip322 commented Oct 13, 2018

ethanhs left a comment

chrisphilip322 commented Oct 14, 2018

gvanrossum commented Oct 14, 2018

gvanrossum left a comment •

edited

Loading

gvanrossum Oct 14, 2018

gvanrossum Oct 14, 2018

gvanrossum Oct 14, 2018

gvanrossum Oct 14, 2018

gvanrossum commented Oct 14, 2018

chrisphilip322 commented Oct 14, 2018

gvanrossum commented Oct 14, 2018

chrisphilip322 commented Oct 14, 2018

gvanrossum commented Oct 14, 2018

ilevkivskyi commented Oct 14, 2018

ilevkivskyi commented Oct 14, 2018

gvanrossum commented Oct 14, 2018

ilevkivskyi commented Oct 14, 2018 •

edited

Loading

gvanrossum commented Oct 14, 2018

gvanrossum commented Oct 14, 2018

gvanrossum left a comment

gvanrossum Oct 15, 2018

ethanhs left a comment

ethanhs Oct 16, 2018

ethanhs Oct 16, 2018

ethanhs Oct 16, 2018

gvanrossum left a comment

		return _NAMESPACE_PROGRAM.format(import_style=import_style.value)


		class ExampleProg(object):

Refactor testpep561 and add test to address #5767 #5782

Refactor testpep561 and add test to address #5767 #5782

Conversation

chrisphilip322 commented Oct 12, 2018

gvanrossum commented Oct 12, 2018

gvanrossum commented Oct 12, 2018

chrisphilip322 commented Oct 12, 2018

chrisphilip322 commented Oct 13, 2018

gvanrossum commented Oct 13, 2018

chrisphilip322 commented Oct 13, 2018

ethanhs left a comment

Choose a reason for hiding this comment

chrisphilip322 commented Oct 14, 2018

gvanrossum commented Oct 14, 2018

gvanrossum left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum commented Oct 14, 2018

chrisphilip322 commented Oct 14, 2018

gvanrossum commented Oct 14, 2018

chrisphilip322 commented Oct 14, 2018

gvanrossum commented Oct 14, 2018

ilevkivskyi commented Oct 14, 2018

ilevkivskyi commented Oct 14, 2018

gvanrossum commented Oct 14, 2018

ilevkivskyi commented Oct 14, 2018 • edited Loading

gvanrossum commented Oct 14, 2018

gvanrossum commented Oct 14, 2018

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanhs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

gvanrossum left a comment •

edited

Loading

ilevkivskyi commented Oct 14, 2018 •

edited

Loading