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

get_type_hints() fails on Mark.__init__ #3635

Closed
Vlad-Shcherbina opened this Issue Jun 28, 2018 · 11 comments

Comments

Projects
None yet
5 participants
@Vlad-Shcherbina
Copy link
Contributor

Vlad-Shcherbina commented Jun 28, 2018

To reproduce, run the following snippet:

import typing
import _pytest.mark
print(typing.get_type_hints(_pytest.mark.Mark.__init__))

Expected:

{'return': <class 'NoneType'>, 'name': <class 'str'>, 'args': typing.List[object], 'kwargs': typing.Dict[str, object]}

Actual:

Traceback (most recent call last):
  File ".\example.py", line 3, in <module>
    print(typing.get_type_hints(_pytest.mark.Mark.__init__))
  File "C:\Python37\lib\typing.py", line 1001, in get_type_hints
    value = _eval_type(value, globalns, localns)
  File "C:\Python37\lib\typing.py", line 260, in _eval_type
    return t._evaluate(globalns, localns)
  File "C:\Python37\lib\typing.py", line 464, in _evaluate
    eval(self.__forward_code__, globalns, localns),
  File "<string>", line 1, in <module>
NameError: name 'List' is not defined

This happens because of these type annotations:

@attr.s(frozen=True)
class Mark(object):
#: name of the mark
name = attr.ib(type=str)
#: positional arguments of the mark decorator
args = attr.ib(type="List[object]")
#: keyword arguments of the mark decorator
kwargs = attr.ib(type="Dict[str, object]")

It's probably a minor issue that has no effect on the normal operation of pytest, but it could trip up runtime annotation analysis tools.

If the codebase was Python 3 only, the fix would be trivial:

from typing import List, Dict
...
@attr.s(frozen=True)
class Mark(object):
    name = attr.ib(type=str)
    args = attr.ib(type=List[object])
    kwargs = attr.ib(type=Dict[str, object])

But I'm not sure how to proceed in Python 2/3 codebase, adding the conditionals does not seem justified for such a tiny thing.


Version info:

  • Python 3.7.0 (v3.7.0:1bf9cc5093, Jun 27 2018, 04:59:51) [MSC v.1914 64 bit (AMD64)] on win32
  • pytest 3.6.2 or pytest 3.6.3.dev33+ga48c47b5
@pytestbot

This comment has been minimized.

Copy link

pytestbot commented Jun 28, 2018

GitMate.io thinks possibly related issues are #1164 (test_something fails), #74 (test_cmdline_python_package() fails), #1852 (test_doctest.py fails ), #3048 (Pytest fails on marking of input parameter if it is a method of a class), and #2872 (mark fixtures ).

@pytestbot pytestbot added the type: bug label Jun 28, 2018

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jun 29, 2018

Thanks @Vlad-Shcherbina for the report!

But I'm not sure how to proceed in Python 2/3 codebase, adding the conditionals does not seem justified for such a tiny thing.

The attr.ib docs say:

The type of the attribute. In Python 3.6 or greater, the preferred method to specify the type is using a variable annotation (see PEP 526). This argument is provided for backward compatibility. Regardless of the approach used, the type will be stored on Attribute.type.

So right now it seems the type= is only serving as the documentation, so it could be changed to:

@attr.s(frozen=True)
class Mark(object):
    name = attr.ib(type=str)
    args = attr.ib()  # type: List[object]
    kwargs = attr.ib()  # type: Dict[str, object]

@RonnyPfannschmidt what do you think?

Perhaps @hynek has another suggestion?

@hynek

This comment has been minimized.

Copy link
Contributor

hynek commented Jun 29, 2018

Firstly to b clear, it’s not necessarily just documentation since it’s metadata that is attached to the attributes that can be used by others – others being mostly mypy nowadays:

import attr

@attr.s
class X:
    x = attr.ib(type=int)

X("x")

leads to t.py:7: error: Argument 1 to "X" has incompatible type "str"; expected "int"


But it Is a non-standard way of defining types nonetheless, so I would expect type annotations to work better. Mypy knows about type= because it has an attrs plugin – CPython proper is blissfully ignorant. :)

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Jun 29, 2018

typing has a backport, how about just using it?

@Vlad-Shcherbina

This comment has been minimized.

Copy link
Contributor Author

Vlad-Shcherbina commented Jun 29, 2018

If you are ok with adding extra dependency, I will prepare a PR.

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Jun 29, 2018

main question is how expensive is importing it

Vlad-Shcherbina added a commit to Vlad-Shcherbina/pytest that referenced this issue Jun 29, 2018

@Vlad-Shcherbina

This comment has been minimized.

Copy link
Contributor Author

Vlad-Shcherbina commented Jun 29, 2018

It's a single 2KLOC file that imports some standard modules that are likely in the import cache already, defines a number of classes, and creates a few objects. So probably not too expensive.

from time import time
start = time()
import typing
print(time() - start)  # ~0.01s on my machine

Vlad-Shcherbina added a commit to Vlad-Shcherbina/pytest that referenced this issue Jun 29, 2018

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jun 29, 2018

Hi @hynek,

Firstly to b clear, it’s not necessarily just documentation since it’s metadata that is attached to the attributes that can be used by others

Thanks for the clarification.

typing has a backport, how about just using it?

I'm a little worried about adding typing as dependency, because according to PEP-560:

The typing module is one of the heaviest and slowest modules in the standard library even with all the optimizations made. Mainly this is because subscripted generic types (see PEP 484 for definition of terms used in this PEP) are class objects (see also [1])

So much that they have made it 7 times faster in Python 3.7:

As a result of PEP 560 work, the import time of typing has been reduced by a factor of 7, and many typing operations are now faster.

I would rather use just the comments instead of adding yet another dependency which slows down pytest startup. We can always just use the typing module later when we drop 2.7 (it will be a matter of find/replace to convert from comment-based to the class-based annotations).

Vlad-Shcherbina added a commit to Vlad-Shcherbina/pytest that referenced this issue Jun 29, 2018

@hynek

This comment has been minimized.

Copy link
Contributor

hynek commented Jul 14, 2018

Importing typing is indeed very slow. We do all kinds of crazy shit in attrs to avoid that cost and I believe so did data classes (unless they removed it after the changes @nicoddemus mentioned).

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jul 14, 2018

Thanks for the comment @hynek.

I vote to change to type comments then, at least until we drop Python 2 next year or so. @Vlad-Shcherbina what do you think? Also if you agree, would you be willing to submit a PR? 😁

@Vlad-Shcherbina

This comment has been minimized.

Copy link
Contributor Author

Vlad-Shcherbina commented Jul 14, 2018

No objections to the type comments. I'll make a PR.

Vlad-Shcherbina added a commit to Vlad-Shcherbina/pytest that referenced this issue Jul 15, 2018

cmccandless added a commit to cmccandless/tools that referenced this issue Jul 30, 2018

Update pytest to 3.6.4 (#22)
This PR updates [pytest](https://pypi.org/project/pytest) from **3.6.3** to **3.6.4**.



<details>
  <summary>Changelog</summary>
  
  
   ### 3.6.4
   ```
   =========================

Bug Fixes
---------

- Invoke pytest using ``-mpytest`` so ``sys.path`` does not get polluted by packages installed in ``site-packages``. (`742 &lt;https://github.com/pytest-dev/pytest/issues/742&gt;`_)


Improved Documentation
----------------------

- Use ``smtp_connection`` instead of ``smtp`` in fixtures documentation to avoid possible confusion. (`3592 &lt;https://github.com/pytest-dev/pytest/issues/3592&gt;`_)


Trivial/Internal Changes
------------------------

- Remove obsolete ``__future__`` imports. (`2319 &lt;https://github.com/pytest-dev/pytest/issues/2319&gt;`_)

- Add CITATION to provide information on how to formally cite pytest. (`3402 &lt;https://github.com/pytest-dev/pytest/issues/3402&gt;`_)

- Replace broken type annotations with type comments. (`3635 &lt;https://github.com/pytest-dev/pytest/issues/3635&gt;`_)

- Pin ``pluggy`` to ``&lt;0.8``. (`3727 &lt;https://github.com/pytest-dev/pytest/issues/3727&gt;`_)
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pytest
  - Changelog: https://pyup.io/changelogs/pytest/
  - Repo: https://github.com/pytest-dev/pytest/issues
  - Homepage: http://pytest.org
</details>

jezdez added a commit to mozilla/telemetry-analysis-service that referenced this issue Aug 14, 2018

Update pytest to 3.7.1 (#1247)
This PR updates [pytest](https://pypi.org/project/pytest) from **3.6.2** to **3.7.1**.



<details>
  <summary>Changelog</summary>
  
  
   ### 3.7.1
   ```
   =========================

Bug Fixes
---------

- `3473 &lt;https://github.com/pytest-dev/pytest/issues/3473&gt;`_: Raise immediately if ``approx()`` is given an expected value of a type it doesn&#39;t understand (e.g. strings, nested dicts, etc.).


- `3712 &lt;https://github.com/pytest-dev/pytest/issues/3712&gt;`_: Correctly represent the dimensions of an numpy array when calling ``repr()`` on ``approx()``.

- `3742 &lt;https://github.com/pytest-dev/pytest/issues/3742&gt;`_: Fix incompatibility with third party plugins during collection, which produced the error ``object has no attribute &#39;_collectfile&#39;``.

- `3745 &lt;https://github.com/pytest-dev/pytest/issues/3745&gt;`_: Display the absolute path if ``cache_dir`` is not relative to the ``rootdir`` instead of failing.


- `3747 &lt;https://github.com/pytest-dev/pytest/issues/3747&gt;`_: Fix compatibility problem with plugins and the warning code issued by fixture functions when they are called directly.


- `3748 &lt;https://github.com/pytest-dev/pytest/issues/3748&gt;`_: Fix infinite recursion in ``pytest.approx`` with arrays in ``numpy&lt;1.13``.


- `3757 &lt;https://github.com/pytest-dev/pytest/issues/3757&gt;`_: Pin pathlib2 to ``&gt;=2.2.0`` as we require ``__fspath__`` support.


- `3763 &lt;https://github.com/pytest-dev/pytest/issues/3763&gt;`_: Fix ``TypeError`` when the assertion message is ``bytes`` in python 3.
   ```
   
  
  
   ### 3.7.0
   ```
   =========================

Deprecations and Removals
-------------------------

- `2639 &lt;https://github.com/pytest-dev/pytest/issues/2639&gt;`_: ``pytest_namespace`` has been deprecated.

  See the documentation for ``pytest_namespace`` hook for suggestions on how to deal
  with this in plugins which use this functionality.


- `3661 &lt;https://github.com/pytest-dev/pytest/issues/3661&gt;`_: Calling a fixture function directly, as opposed to request them in a test function, now issues a ``RemovedInPytest4Warning``. It will be changed into an error in pytest ``4.0``.

  This is a great source of confusion to new users, which will often call the fixture functions and request them from test functions interchangeably, which breaks the fixture resolution model.



Features
--------

- `2283 &lt;https://github.com/pytest-dev/pytest/issues/2283&gt;`_: New ``package`` fixture scope: fixtures are finalized when the last test of a *package* finishes. This feature is considered **experimental**, so use it sparingly.


- `3576 &lt;https://github.com/pytest-dev/pytest/issues/3576&gt;`_: ``Node.add_marker`` now supports an ``append=True/False`` parameter to determine whether the mark comes last (default) or first.


- `3579 &lt;https://github.com/pytest-dev/pytest/issues/3579&gt;`_: Fixture ``caplog`` now has a ``messages`` property, providing convenient access to the format-interpolated log messages without the extra data provided by the formatter/handler.


- `3610 &lt;https://github.com/pytest-dev/pytest/issues/3610&gt;`_: New ``--trace`` option to enter the debugger at the start of a test.


- `3623 &lt;https://github.com/pytest-dev/pytest/issues/3623&gt;`_: Introduce ``pytester.copy_example`` as helper to do acceptance tests against examples from the project.



Bug Fixes
---------

- `2220 &lt;https://github.com/pytest-dev/pytest/issues/2220&gt;`_: Fix a bug where fixtures overridden by direct parameters (for example parametrization) were being instantiated even if they were not being used by a test.


- `3695 &lt;https://github.com/pytest-dev/pytest/issues/3695&gt;`_: Fix ``ApproxNumpy`` initialisation argument mixup, ``abs`` and ``rel`` tolerances were flipped causing strange comparsion results.
  Add tests to check ``abs`` and ``rel`` tolerances for ``np.array`` and test for expecting ``nan`` with ``np.array()``


- `980 &lt;https://github.com/pytest-dev/pytest/issues/980&gt;`_: Fix truncated locals output in verbose mode.



Improved Documentation
----------------------

- `3295 &lt;https://github.com/pytest-dev/pytest/issues/3295&gt;`_: Correct the usage documentation of ``--last-failed-no-failures`` by adding the missing ``--last-failed`` argument in the presented examples, because they are misleading and lead to think that the missing argument is not needed.



Trivial/Internal Changes
------------------------

- `3519 &lt;https://github.com/pytest-dev/pytest/issues/3519&gt;`_: Now a ``README.md`` file is created in ``.pytest_cache`` to make it clear why the directory exists.
   ```
   
  
  
   ### 3.6.4
   ```
   =========================

Bug Fixes
---------

- Invoke pytest using ``-mpytest`` so ``sys.path`` does not get polluted by packages installed in ``site-packages``. (`742 &lt;https://github.com/pytest-dev/pytest/issues/742&gt;`_)


Improved Documentation
----------------------

- Use ``smtp_connection`` instead of ``smtp`` in fixtures documentation to avoid possible confusion. (`3592 &lt;https://github.com/pytest-dev/pytest/issues/3592&gt;`_)


Trivial/Internal Changes
------------------------

- Remove obsolete ``__future__`` imports. (`2319 &lt;https://github.com/pytest-dev/pytest/issues/2319&gt;`_)

- Add CITATION to provide information on how to formally cite pytest. (`3402 &lt;https://github.com/pytest-dev/pytest/issues/3402&gt;`_)

- Replace broken type annotations with type comments. (`3635 &lt;https://github.com/pytest-dev/pytest/issues/3635&gt;`_)

- Pin ``pluggy`` to ``&lt;0.8``. (`3727 &lt;https://github.com/pytest-dev/pytest/issues/3727&gt;`_)
   ```
   
  
  
   ### 3.6.3
   ```
   =========================

Bug Fixes
---------

- Fix ``ImportWarning`` triggered by explicit relative imports in
  assertion-rewritten package modules. (`3061
  &lt;https://github.com/pytest-dev/pytest/issues/3061&gt;`_)

- Fix error in ``pytest.approx`` when dealing with 0-dimension numpy
  arrays. (`3593 &lt;https://github.com/pytest-dev/pytest/issues/3593&gt;`_)

- No longer raise ``ValueError`` when using the ``get_marker`` API. (`3605
  &lt;https://github.com/pytest-dev/pytest/issues/3605&gt;`_)

- Fix problem where log messages with non-ascii characters would not
  appear in the output log file.
  (`3630 &lt;https://github.com/pytest-dev/pytest/issues/3630&gt;`_)

- No longer raise ``AttributeError`` when legacy marks can&#39;t be stored in
  functions. (`3631 &lt;https://github.com/pytest-dev/pytest/issues/3631&gt;`_)


Improved Documentation
----------------------

- The description above the example for ``pytest.mark.skipif`` now better
  matches the code. (`3611
  &lt;https://github.com/pytest-dev/pytest/issues/3611&gt;`_)


Trivial/Internal Changes
------------------------

- Internal refactoring: removed unused ``CallSpec2tox ._globalid_args``
  attribute and ``metafunc`` parameter from ``CallSpec2.copy()``. (`3598
  &lt;https://github.com/pytest-dev/pytest/issues/3598&gt;`_)

- Silence usage of ``reduce`` warning in Python 2 (`3609
  &lt;https://github.com/pytest-dev/pytest/issues/3609&gt;`_)

- Fix usage of ``attr.ib`` deprecated ``convert`` parameter. (`3653
  &lt;https://github.com/pytest-dev/pytest/issues/3653&gt;`_)
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pytest
  - Changelog: https://pyup.io/changelogs/pytest/
  - Repo: https://github.com/pytest-dev/pytest/issues
  - Homepage: http://pytest.org
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.