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

introduce a distinct searchable non-broken storage for markers #3317

Merged
merged 27 commits into from Apr 9, 2018

Conversation

Projects
7 participants
@RonnyPfannschmidt
Member

RonnyPfannschmidt commented Mar 16, 2018

as part of this i will deprecate markinfo,

there will be much to change about the internals,
but the find_markers api that from now on will work across all node level instead of just function will sort out some of the biggest issues

@coveralls

This comment has been minimized.

coveralls commented Mar 16, 2018

Coverage Status

Coverage increased (+0.02%) to 92.828% when pulling 4df8f2b on RonnyPfannschmidt:marker-pristine-node-storage into 2962c73 on pytest-dev:features.

@RonnyPfannschmidt RonnyPfannschmidt requested a review from nicoddemus Mar 16, 2018

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Mar 16, 2018

the current state of the pr is that mark evaluation is ported, some bugs magically disappeared due to no longer having a confusing base structure

whats missing is adding tests for the old strucutre and deciding how to proceed with the old structure of the keyword mappings

whats also missing is deprecating mark-info usage and adding an option to disable marker transfer alltogether

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Mar 16, 2018

whats also missing is deprecating mark-info usage and adding an option to disable marker transfer alltogether

How do you plan to make that option available to users? Class/module based, ini-option?

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Mar 16, 2018

@RonnyPfannschmidt thanks for the ongoing effort to clean up marks! 😁

At a glance this looks good, could you draft some docs as well, including examples? This will help trying to understand the changes and to criticize the API.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Mar 16, 2018

Also because this introduces a core and important new API, perhaps you can write up an email highlighting the problems of the old API and showing up examples of the new API? We should bring more people to the discussion.

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Mar 17, 2018

@nicoddemus the api was part of the discussion we did on the ml a few weeks ago - my changes are bascially according to plan, with the caveat that the introduction of a functiondefinition node is not going as well as i'd like

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Mar 17, 2018

current state - due to the metafunc spaghetti this is turning into a MAJOR refactor, as far as i can tell we will have to leave things incorrect until metafunc is sorted

@RonnyPfannschmidt RonnyPfannschmidt changed the title from [wip] introduce a distinct searchable non-broken storage for markers to introduce a distinct searchable non-broken storage for markers Mar 19, 2018

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Mar 19, 2018

@nicoddemus i believe this one needs now a bit more communication

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Mar 19, 2018

upcoming is a cset wrt get_marker issues

@fschulze

This comment has been minimized.

Contributor

fschulze commented Mar 20, 2018

I just tried this branch with devpi-server, which (ab)uses markers heavily. The tests all work, but as expected I noticed some small changes due to the changes in the transfer semantics. In our case that's a good thing though, because we want explicit markers and not implicit transfers. So 👍 from my side so far, though I can say nothing about the implementation itself.

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Mar 20, 2018

@fschulze first thanks for trying and experimenting 👍

second - can you show me the changes, just so i can verify on whether this is a problem or an enhancement, and if there is extra needs for documentation?

@@ -178,15 +179,23 @@ def add_marker(self, marker):
elif not isinstance(marker, MarkDecorator):
raise ValueError("is not a string or pytest.mark.* Marker")
self.keywords[marker.name] = marker
self._markers.update([marker])
def find_markers(self, name):

This comment has been minimized.

@flub

flub Mar 20, 2018

Member

Does this automatically make it to the reference docs because the whole class is autodoc'd?

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Mar 20, 2018

Member

i beleive so

im planning to work on the docs a bit more

"""find all marks with the given name on the node and its parents
:param str name: name of the marker
"""

This comment has been minimized.

@flub

flub Mar 20, 2018

Member

for the reference docs I'd like to mention that this is a generator yielding the marks instead of the very vague "find all marks"

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Mar 20, 2018

Member

fixed, thanks for the note

:param str name: name of the marker
"""
for node in reversed(self.listchain()):
for mark in node._markers.find(name):

This comment has been minimized.

@flub

flub Mar 20, 2018

Member

Seems like I can only iterate when I know what I'm looking for. Can we allow iterating over all marks as well?

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Mar 20, 2018

Member

with the current mark mechanism you dont really get that sane iteration as well, so i dodnt want to add an api since apis are easy to get wrong

as for mamrkers - i would like to subscribe them to the "you better know what you are looking for" creed

i am currently against adding the arbitrary iteration unless someone can demonstrate a real use-case

@nicoddemus

So far it looks great @RonnyPfannschmidt.

Some small comments, but I think the most important one is comprehensive docs about the change. 👍

@@ -977,10 +975,10 @@ def getfixtureinfo(self, node, func, cls, funcargs=True):
argnames = getfuncargnames(func, cls=cls)
else:
argnames = ()
usefixtures = getattr(func, "usefixtures", None)
usefixtures = flatten(uf_mark.args for uf_mark in node.find_markers("usefixtures"))
initialnames = argnames
if usefixtures is not None:

This comment has been minimized.

@nicoddemus

nicoddemus Mar 20, 2018

Member

This if can be removed given that flatten always returns an iterator.

@@ -1055,6 +1053,8 @@ def pytest_generate_tests(self, metafunc):
fixturedef = faclist[-1]
if fixturedef.params is not None:
parametrize_func = getattr(metafunc.function, 'parametrize', None)

This comment has been minimized.

@nicoddemus

nicoddemus Mar 20, 2018

Member

Should this use find_markers as well?

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Mar 21, 2018

Member

yes and no, im at it now and its a very brittle part of fixtures i dont fully understand,
its currently implemented using the old mechanism, and i'm afraid to touch it as its so messy

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Mar 21, 2018

Member

i tracked it back to c462393

as far as i can tell it has been incorrect the whole time and i cant fix that incorrectness, but i should use find_markers

im on it now

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Mar 21, 2018

Member

about 3 different attempts to fox this with a normal loop failed, its not clear how to correctly fix at first glance, as such i will leave that code as is for now and revisit later

@attr.s(cmp=False, hash=False)
class NodeMarkers(object):
"""
internal strucutre for storing marks belongong to a node

This comment has been minimized.

@nicoddemus

nicoddemus Mar 20, 2018

Member

Should this be added to the docs in reference.rst?

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Mar 21, 2018

Member

i think so, it should be noted that it currently cant be accessed via public attributes

This comment has been minimized.

@flub

flub Mar 21, 2018

Member

If that's the case, then it doesn't need adding to the docs does it?

def obj():
def fget(self):
obj = getattr(self, '_obj', None)
if obj is None:
self._obj = obj = self._getobj()
# XXX evil hack

This comment has been minimized.

@nicoddemus

nicoddemus Mar 20, 2018

Member

please add a comment explaining why this is evil. 😁

@@ -524,6 +534,8 @@ def setup(self):
class Instance(PyCollector):
_ALLOW_MARKERS = False # hack, destroy later

This comment has been minimized.

@nicoddemus

nicoddemus Mar 20, 2018

Member

Please mention where and when do we plan to destroy this.

@@ -0,0 +1 @@
introduce correct per node mark handling and deprecate the always incorrect existing mark handling

This comment has been minimized.

@nicoddemus

nicoddemus Mar 20, 2018

Member

Please add a note to the docs in markers.rst explaining this change and how it affects end users in detail, and post a link to that section here; I think one of the most important things we need to do about this change is to properly communicate what they are, what it solves and how it might affect users. 👍

Introduce correct per-node mark handling, which aims to fix long-standing problems on how marks are used and propagated through objects. For more information please see `this section in the docs <some url>`_.
:returns: iterator over marks matching the name"""
return map(itemgetter(1), self.find_markers_with_node(name))
def find_markers_with_node(self, name):

This comment has been minimized.

@nicoddemus

nicoddemus Mar 26, 2018

Member

I find this name confusing TBH... perhaps it would be better to have just one find_markers function which always iterates over (node, mark) tuples?

This comment has been minimized.

@nicoddemus

nicoddemus Mar 26, 2018

Member

Another idea: return a namedtuple instead, with node and mark attributes. It is almost the same, but let's us incrementally add more things to the iteration items if we need in the future.

for mark_info in find_markers('skip'):
    if mark_info.node == ... and mark_info.skip.args == ?

We just need a better name than mark_info.

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Mar 27, 2018

Member

mark_info was always a dead name,

having a "merged" object for mark and node is going to be a major pain,

the loop example you posted is a unacceptable mush

This comment has been minimized.

@nicoddemus

nicoddemus Mar 27, 2018

Member

TBH I'm not a big fan of the last example either, just trying to post alternatives. Not sure why you must use "unacceptable mush" to describe it though, because the same loop under your API is:

for (node, mark) in find_markers_with_node('skip'):
    if node.nodeid == ... and mark.args == ...:
def iter_markers(self):
"""
iterate over all markers of the node
"""
return chain.from_iterable(x._markers for x in reversed(self.listchain()))
def iter_markers_with_node(self):

This comment has been minimized.

@nicoddemus

nicoddemus Mar 26, 2018

Member

Same comment as before, the name seems confusing. Again it seems having a single version would be less confusing (returning an iterator of (node, mark)).

We might even consider to drop the "find by name" versions and let users do the search themselves; this is very easy to do with a list/generator comprehension. We can always add more API later, but removing them is always problematic as we know.

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Mar 27, 2018

Member

i would like to introduce those apis as experimental and leave them as such for at least 6 months

This comment has been minimized.

@flub

flub Mar 27, 2018

Member

I can also be found for having fewer variations of this available. A minimalistic API would just be iter_markers which yields the (node, mark) tuples, is that a nice starting point? It's pretty easy for the user to write the filter. Especially since the implementations provided here don't provide anything major, it's not like find_markers is much more efficient then iter_markers with your own filter.

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt and others added some commits Apr 5, 2018

@nicoddemus nicoddemus merged commit 7153370 into pytest-dev:features Apr 9, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Apr 9, 2018

@RonnyPfannschmidt merged, thanks again for all the hard work!

Btw, could you search for mark related issues and close the ones that are fixed by this merge? Thanks!

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Apr 10, 2018

wohoo :) i have a complete project - its about 20

@theY4Kman

This comment has been minimized.

theY4Kman commented Apr 19, 2018

giphy 12

Thank you for this! I was forced to use -k overly_specific_filter instead of -m only due to heavy reliance on inheritance. Now I'm free! Freeeeeeeeeee!!!

@fschulze

This comment has been minimized.

Contributor

fschulze commented May 23, 2018

@RonnyPfannschmidt sorry for the far too late reply, but here are the changes I had to make in devpi-server: devpi/devpi@be961ab Basically before this PR module level markers were transferred to tests imported in another module and now I had to explicitly add the module level markers where the tests are imported. That case is very much on the edge and it was no problem. It's definitely better now than it was before. I have some cases where the slow marker was transferred back to the base class where it didn't belong and that is now fixed.

Thanks for the work!

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented May 23, 2018

@fschulze i see - this was part of the bug then - the markers would be bluntly be smeared into random other objects - it should never have worked to begin with

guykisel added a commit to guykisel/inline-plz that referenced this pull request Jul 6, 2018

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



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

Bug Fixes
---------

- Fix regression in ``Node.add_marker`` by extracting the mark object of a
  ``MarkDecorator``. (`3555
  &lt;https://github.com/pytest-dev/pytest/issues/3555&gt;`_)

- Warnings without ``location`` were reported as ``None``. This is corrected to
  now report ``&lt;undetermined location&gt;``. (`3563
  &lt;https://github.com/pytest-dev/pytest/issues/3563&gt;`_)

- Continue to call finalizers in the stack when a finalizer in a former scope
  raises an exception. (`3569
  &lt;https://github.com/pytest-dev/pytest/issues/3569&gt;`_)

- Fix encoding error with `print` statements in doctests (`3583
  &lt;https://github.com/pytest-dev/pytest/issues/3583&gt;`_)


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

- Add documentation for the ``--strict`` flag. (`3549
  &lt;https://github.com/pytest-dev/pytest/issues/3549&gt;`_)


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

- Update old quotation style to parens in fixture.rst documentation. (`3525
  &lt;https://github.com/pytest-dev/pytest/issues/3525&gt;`_)

- Improve display of hint about ``--fulltrace`` with ``KeyboardInterrupt``.
  (`3545 &lt;https://github.com/pytest-dev/pytest/issues/3545&gt;`_)

- pytest&#39;s testsuite is no longer runnable through ``python setup.py test`` --
  instead invoke ``pytest`` or ``tox`` directly. (`3552
  &lt;https://github.com/pytest-dev/pytest/issues/3552&gt;`_)

- Fix typo in documentation (`3567
  &lt;https://github.com/pytest-dev/pytest/issues/3567&gt;`_)
   ```
   
  
  
   ### 3.6.1
   ```
   =========================

Bug Fixes
---------

- Fixed a bug where stdout and stderr were logged twice by junitxml when a test
  was marked xfail. (`3491
  &lt;https://github.com/pytest-dev/pytest/issues/3491&gt;`_)

- Fix ``usefixtures`` mark applyed to unittest tests by correctly instantiating
  ``FixtureInfo``. (`3498
  &lt;https://github.com/pytest-dev/pytest/issues/3498&gt;`_)

- Fix assertion rewriter compatibility with libraries that monkey patch
  ``file`` objects. (`3503
  &lt;https://github.com/pytest-dev/pytest/issues/3503&gt;`_)


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

- Added a section on how to use fixtures as factories to the fixture
  documentation. (`3461 &lt;https://github.com/pytest-dev/pytest/issues/3461&gt;`_)


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

- Enable caching for pip/pre-commit in order to reduce build time on
  travis/appveyor. (`3502
  &lt;https://github.com/pytest-dev/pytest/issues/3502&gt;`_)

- Switch pytest to the src/ layout as we already suggested it for good practice
  - now we implement it as well. (`3513
  &lt;https://github.com/pytest-dev/pytest/issues/3513&gt;`_)

- Fix if in tests to support 3.7.0b5, where a docstring handling in AST got
  reverted. (`3530 &lt;https://github.com/pytest-dev/pytest/issues/3530&gt;`_)

- Remove some python2.5 compatibility code. (`3529
  &lt;https://github.com/pytest-dev/pytest/issues/3529&gt;`_)
   ```
   
  
  
   ### 3.6.0
   ```
   =========================

Features
--------

- Revamp the internals of the ``pytest.mark`` implementation with correct per
  node handling which fixes a number of long standing bugs caused by the old
  design. This introduces new ``Node.iter_markers(name)`` and
  ``Node.get_closest_mark(name)`` APIs. Users are **strongly encouraged** to
  read the `reasons for the revamp in the docs
  &lt;https://docs.pytest.org/en/latest/mark.htmlmarker-revamp-and-iteration&gt;`_,
  or jump over to details about `updating existing code to use the new APIs
  &lt;https://docs.pytest.org/en/latest/mark.htmlupdating-code&gt;`_. (`3317
  &lt;https://github.com/pytest-dev/pytest/issues/3317&gt;`_)

- Now when ``pytest.fixture`` is applied more than once to the same function a
  ``ValueError`` is raised. This buggy behavior would cause surprising problems
  and if was working for a test suite it was mostly by accident. (`2334
  &lt;https://github.com/pytest-dev/pytest/issues/2334&gt;`_)

- Support for Python 3.7&#39;s builtin ``breakpoint()`` method, see `Using the
  builtin breakpoint function
  &lt;https://docs.pytest.org/en/latest/usage.htmlbreakpoint-builtin&gt;`_ for
  details. (`3180 &lt;https://github.com/pytest-dev/pytest/issues/3180&gt;`_)

- ``monkeypatch`` now supports a ``context()`` function which acts as a context
  manager which undoes all patching done within the ``with`` block. (`3290
  &lt;https://github.com/pytest-dev/pytest/issues/3290&gt;`_)

- The ``--pdb`` option now causes KeyboardInterrupt to enter the debugger,
  instead of stopping the test session. On python 2.7, hitting CTRL+C again
  exits the debugger. On python 3.2 and higher, use CTRL+D. (`3299
  &lt;https://github.com/pytest-dev/pytest/issues/3299&gt;`_)

- pytest not longer changes the log level of the root logger when the
  ``log-level`` parameter has greater numeric value than that of the level of
  the root logger, which makes it play better with custom logging configuration
  in user code. (`3307 &lt;https://github.com/pytest-dev/pytest/issues/3307&gt;`_)


Bug Fixes
---------

- A rare race-condition which might result in corrupted ``.pyc`` files on
  Windows has been hopefully solved. (`3008
  &lt;https://github.com/pytest-dev/pytest/issues/3008&gt;`_)

- Also use iter_marker for discovering the marks applying for marker
  expressions from the cli to avoid the bad data from the legacy mark storage.
  (`3441 &lt;https://github.com/pytest-dev/pytest/issues/3441&gt;`_)

- When showing diffs of failed assertions where the contents contain only
  whitespace, escape them using ``repr()`` first to make it easy to spot the
  differences. (`3443 &lt;https://github.com/pytest-dev/pytest/issues/3443&gt;`_)


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

- Change documentation copyright year to a range which auto-updates itself each
  time it is published. (`3303
  &lt;https://github.com/pytest-dev/pytest/issues/3303&gt;`_)


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

- ``pytest`` now depends on the `python-atomicwrites
  &lt;https://github.com/untitaker/python-atomicwrites&gt;`_ library. (`3008
  &lt;https://github.com/pytest-dev/pytest/issues/3008&gt;`_)

- Update all pypi.python.org URLs to pypi.org. (`3431
  &lt;https://github.com/pytest-dev/pytest/issues/3431&gt;`_)

- Detect `pytest_` prefixed hooks using the internal plugin manager since
  ``pluggy`` is deprecating the ``implprefix`` argument to ``PluginManager``.
  (`3487 &lt;https://github.com/pytest-dev/pytest/issues/3487&gt;`_)

- Import ``Mapping`` and ``Sequence`` from ``_pytest.compat`` instead of
  directly from ``collections`` in ``python_api.py::approx``. Add ``Mapping``
  to ``_pytest.compat``, import it from ``collections`` on python 2, but from
  ``collections.abc`` on Python 3 to avoid a ``DeprecationWarning`` on Python
  3.7 or newer. (`3497 &lt;https://github.com/pytest-dev/pytest/issues/3497&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>

@RonnyPfannschmidt RonnyPfannschmidt deleted the RonnyPfannschmidt:marker-pristine-node-storage branch Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment