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

RonnyPfannschmidt · 2018-03-16T10:04:49Z

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 · 2018-03-16T10:34:29Z

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

RonnyPfannschmidt · 2018-03-16T10:37:57Z

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 · 2018-03-16T21:41:39Z

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 · 2018-03-16T21:43:08Z

@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 · 2018-03-16T21:44:19Z

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 · 2018-03-17T08:25:59Z

@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 · 2018-03-17T20:37:04Z

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 · 2018-03-19T16:50:13Z

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

RonnyPfannschmidt · 2018-03-19T17:08:46Z

upcoming is a cset wrt get_marker issues

fschulze · 2018-03-20T12:24:39Z

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 · 2018-03-20T13:07:07Z

@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?

flub · 2018-03-20T14:02:15Z

_pytest/nodes.py

@@ -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):


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

i beleive so

im planning to work on the docs a bit more

flub · 2018-03-20T14:03:36Z

_pytest/nodes.py

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


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"

fixed, thanks for the note

flub · 2018-03-20T14:05:48Z

_pytest/nodes.py

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


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

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

nicoddemus · 2018-03-20T21:48:53Z

_pytest/fixtures.py

@@ -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 if can be removed given that flatten always returns an iterator.

nicoddemus · 2018-03-20T21:49:28Z

_pytest/fixtures.py

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


Should this use find_markers as well?

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

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

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

nicoddemus · 2018-03-20T22:03:00Z

_pytest/mark/structures.py

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


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

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

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

nicoddemus · 2018-03-20T22:05:22Z

_pytest/python.py

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


please add a comment explaining why this is evil. 😁

nicoddemus · 2018-03-20T22:07:43Z

_pytest/python.py

@@ -524,6 +534,8 @@ def setup(self):


 class Instance(PyCollector):
+    _ALLOW_MARKERS = False  # hack, destroy later


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

nicoddemus · 2018-03-20T22:12:26Z

changelog/3317.feature

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


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>`_.

nicoddemus · 2018-03-26T23:51:26Z

_pytest/nodes.py

+        :returns: iterator over marks matching the name"""
+        return map(itemgetter(1), self.find_markers_with_node(name))
+
+    def find_markers_with_node(self, name):


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?

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.

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

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 == ...:

nicoddemus · 2018-03-26T23:54:44Z

_pytest/nodes.py


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


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.

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

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.

ok, will do

…tely breaking metafunc testing

…arks

…m node.get_marker

nicoddemus · 2018-04-09T22:40:40Z

@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 · 2018-04-10T04:25:08Z

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

theY4Kman · 2018-04-19T18:52:41Z

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 · 2018-05-23T12:16:37Z

@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 · 2018-05-23T12:21:20Z

@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

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 <https://github.com/pytest-dev/pytest/issues/3555>`_) - Warnings without ``location`` were reported as ``None``. This is corrected to now report ``<undetermined location>``. (`3563 <https://github.com/pytest-dev/pytest/issues/3563>`_) - Continue to call finalizers in the stack when a finalizer in a former scope raises an exception. (`3569 <https://github.com/pytest-dev/pytest/issues/3569>`_) - Fix encoding error with `print` statements in doctests (`3583 <https://github.com/pytest-dev/pytest/issues/3583>`_) Improved Documentation ---------------------- - Add documentation for the ``--strict`` flag. (`3549 <https://github.com/pytest-dev/pytest/issues/3549>`_) Trivial/Internal Changes ------------------------ - Update old quotation style to parens in fixture.rst documentation. (`3525 <https://github.com/pytest-dev/pytest/issues/3525>`_) - Improve display of hint about ``--fulltrace`` with ``KeyboardInterrupt``. (`3545 <https://github.com/pytest-dev/pytest/issues/3545>`_) - pytest's testsuite is no longer runnable through ``python setup.py test`` -- instead invoke ``pytest`` or ``tox`` directly. (`3552 <https://github.com/pytest-dev/pytest/issues/3552>`_) - Fix typo in documentation (`3567 <https://github.com/pytest-dev/pytest/issues/3567>`_) ``` ### 3.6.1 ``` ========================= Bug Fixes --------- - Fixed a bug where stdout and stderr were logged twice by junitxml when a test was marked xfail. (`3491 <https://github.com/pytest-dev/pytest/issues/3491>`_) - Fix ``usefixtures`` mark applyed to unittest tests by correctly instantiating ``FixtureInfo``. (`3498 <https://github.com/pytest-dev/pytest/issues/3498>`_) - Fix assertion rewriter compatibility with libraries that monkey patch ``file`` objects. (`3503 <https://github.com/pytest-dev/pytest/issues/3503>`_) Improved Documentation ---------------------- - Added a section on how to use fixtures as factories to the fixture documentation. (`3461 <https://github.com/pytest-dev/pytest/issues/3461>`_) Trivial/Internal Changes ------------------------ - Enable caching for pip/pre-commit in order to reduce build time on travis/appveyor. (`3502 <https://github.com/pytest-dev/pytest/issues/3502>`_) - Switch pytest to the src/ layout as we already suggested it for good practice - now we implement it as well. (`3513 <https://github.com/pytest-dev/pytest/issues/3513>`_) - Fix if in tests to support 3.7.0b5, where a docstring handling in AST got reverted. (`3530 <https://github.com/pytest-dev/pytest/issues/3530>`_) - Remove some python2.5 compatibility code. (`3529 <https://github.com/pytest-dev/pytest/issues/3529>`_) ``` ### 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 <https://docs.pytest.org/en/latest/mark.htmlmarker-revamp-and-iteration>`_, or jump over to details about `updating existing code to use the new APIs <https://docs.pytest.org/en/latest/mark.htmlupdating-code>`_. (`3317 <https://github.com/pytest-dev/pytest/issues/3317>`_) - 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 <https://github.com/pytest-dev/pytest/issues/2334>`_) - Support for Python 3.7's builtin ``breakpoint()`` method, see `Using the builtin breakpoint function <https://docs.pytest.org/en/latest/usage.htmlbreakpoint-builtin>`_ for details. (`3180 <https://github.com/pytest-dev/pytest/issues/3180>`_) - ``monkeypatch`` now supports a ``context()`` function which acts as a context manager which undoes all patching done within the ``with`` block. (`3290 <https://github.com/pytest-dev/pytest/issues/3290>`_) - 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 <https://github.com/pytest-dev/pytest/issues/3299>`_) - 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 <https://github.com/pytest-dev/pytest/issues/3307>`_) Bug Fixes --------- - A rare race-condition which might result in corrupted ``.pyc`` files on Windows has been hopefully solved. (`3008 <https://github.com/pytest-dev/pytest/issues/3008>`_) - 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 <https://github.com/pytest-dev/pytest/issues/3441>`_) - 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 <https://github.com/pytest-dev/pytest/issues/3443>`_) Improved Documentation ---------------------- - Change documentation copyright year to a range which auto-updates itself each time it is published. (`3303 <https://github.com/pytest-dev/pytest/issues/3303>`_) Trivial/Internal Changes ------------------------ - ``pytest`` now depends on the `python-atomicwrites <https://github.com/untitaker/python-atomicwrites>`_ library. (`3008 <https://github.com/pytest-dev/pytest/issues/3008>`_) - Update all pypi.python.org URLs to pypi.org. (`3431 <https://github.com/pytest-dev/pytest/issues/3431>`_) - Detect `pytest_` prefixed hooks using the internal plugin manager since ``pluggy`` is deprecating the ``implprefix`` argument to ``PluginManager``. (`3487 <https://github.com/pytest-dev/pytest/issues/3487>`_) - 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 <https://github.com/pytest-dev/pytest/issues/3497>`_) ``` </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>

This class was added and removed during the development of a PR: pytest-dev#3317

This class was both added and became unused during the development of a PR: pytest-dev#3317

RonnyPfannschmidt requested a review from nicoddemus March 16, 2018 10:36

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

RonnyPfannschmidt force-pushed the marker-pristine-node-storage branch from 8b07cb8 to e8c4204 Compare March 19, 2018 18:11

flub reviewed Mar 20, 2018

View reviewed changes

nicoddemus reviewed Mar 20, 2018

View reviewed changes

RonnyPfannschmidt force-pushed the marker-pristine-node-storage branch from 1a457ec to ff04a92 Compare March 26, 2018 17:06

nicoddemus reviewed Mar 26, 2018

View reviewed changes

RonnyPfannschmidt added 11 commits March 29, 2018 17:52

introduce a own storage for markers

360d608

port mark evaluation to the new storage and fix a bug in evaluation

2707221

enable deep merging test - new structure fixed it

f1a1695

fix up the mark evaluator validity check

e8feee0

deprecate markinfo and fix up most marker scoping access while comple…

180ae09

…tely breaking metafunc testing

fix most of metafunc tests by mocking

99015bf

base metafunc fixtureinfo on the functiondefinition to caputure its m…

2d06ae0

…arks

refactor node markers, remove need to be aware of nodes

5e56e9b

add docstrings for nodemarkers

ced1316

first changelog entry

775fb96

turn Markinfo into atts clsas, and return correct instances of it fro…

159ea9b

…m node.get_marker

fix doc build, use noindex on the mark reference

4df8f2b

nicoddemus approved these changes Apr 9, 2018

View reviewed changes

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

This was referenced Apr 10, 2018

Marks don't pick up nested classes #199

Closed

markers stains on all related classes #568

Closed

combining marks - args and kwargs calculation #2897

Closed

Botje mentioned this pull request May 3, 2018

marker expressions use the legacy mark storage #3441

Closed

The-Compiler mentioned this pull request May 4, 2018

Node.get_marker needs a proper replacement #3446

Closed

RonnyPfannschmidt deleted the marker-pristine-node-storage branch October 1, 2018 09:34

Zac-HD mentioned this pull request Oct 21, 2018

mark.usefixtures registered fixtures of all test class subclasses invoked despite -k #2806

Closed

kdelee mentioned this pull request May 2, 2019

AttributeError: 'Function' object has no attribute 'get_closest_marker' jlaska/pytest-github#24

Closed

bluetech added a commit to bluetech/pytest that referenced this pull request Feb 10, 2020

Remove unused NodeMarkers

a89ab20

This class was added and removed during the development of a PR: pytest-dev#3317

bluetech added a commit to bluetech/pytest that referenced this pull request Feb 10, 2020

Remove unused NodeMarkers

7cc513b

This class was both added and became unused during the development of a PR: pytest-dev#3317

bluetech mentioned this pull request Feb 10, 2020

Remove unused NodeMarkers #6710

Merged

		@@ -524,6 +534,8 @@ def setup(self):


		class Instance(PyCollector):
		_ALLOW_MARKERS = False # hack, destroy later

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

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

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

Conversation

RonnyPfannschmidt commented Mar 16, 2018

coveralls commented Mar 16, 2018 • edited Loading

RonnyPfannschmidt commented Mar 16, 2018

nicoddemus commented Mar 16, 2018

nicoddemus commented Mar 16, 2018

nicoddemus commented Mar 16, 2018

RonnyPfannschmidt commented Mar 17, 2018

RonnyPfannschmidt commented Mar 17, 2018

RonnyPfannschmidt commented Mar 19, 2018

RonnyPfannschmidt commented Mar 19, 2018

fschulze commented Mar 20, 2018

RonnyPfannschmidt commented Mar 20, 2018

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

Choose a reason for hiding this comment

nicoddemus 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus Mar 26, 2018 • 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

nicoddemus commented Apr 9, 2018

RonnyPfannschmidt commented Apr 10, 2018

theY4Kman commented Apr 19, 2018 • edited Loading

fschulze commented May 23, 2018

RonnyPfannschmidt commented May 23, 2018

coveralls commented Mar 16, 2018 •

edited

Loading

nicoddemus Mar 26, 2018 •

edited

Loading

theY4Kman commented Apr 19, 2018 •

edited

Loading