fix issue #1618 by considering plugin args first #1673

RonnyPfannschmidt · 2016-06-25T16:33:36Z

additionally prevent unnecessary importation of blocked plugins

The-Compiler · 2016-06-25T21:08:26Z

testing/test_config.py

@@ -352,6 +352,24 @@ def test_inifilename(self, tmpdir):
        assert config.inicfg.get('name') == 'value'
        assert config.inicfg.get('should_not_be_set') is None

+    def test_consider_plugin(self, testdir):
+        pytest.importorskip('xdist')
+        print testdir


Seems like that shouldn't be here

That's what broke all Py3 builds. 😁

nicoddemus · 2016-06-26T18:59:49Z

testing/test_config.py

@@ -352,6 +352,24 @@ def test_inifilename(self, tmpdir):
        assert config.inicfg.get('name') == 'value'
        assert config.inicfg.get('should_not_be_set') is None

+    def test_consider_plugin(self, testdir):


Please mention #1618 on the docstring.

Also, when creating commit messages you can add a line "Fix #1618" so it closes the issue automatically when merged.

additionally prevent unnecessary importation of blocked plugins

Fix pytest-dev#1618

coveralls · 2016-06-27T11:35:22Z

Coverage remained the same at 92.09% when pulling 5f61109 on RonnyPfannschmidt:fix-1618 into ccd395f on pytest-dev:features.

coveralls · 2016-06-27T11:41:27Z

Coverage remained the same at 92.09% when pulling 5f61109 on RonnyPfannschmidt:fix-1618 into ccd395f on pytest-dev:features.

RonnyPfannschmidt · 2016-06-30T08:31:48Z

@The-Compiler @nicoddemus take alook?

nicoddemus · 2016-06-30T08:57:56Z

CHANGELOG.rst

@@ -3,7 +3,7 @@

 **Changes**

-*
+* fix (`#1618`_): properly consider explicitly blocked plugins


Could explain this in more detail here?

I fear this is too "pytest-internal-specific" and users won't really know what's going on... suggestion:

Fix #1618: consider --plugins (-p) option before loading installed plugins, so it can be used to disable
plugins with problems or incorrectly installed (by using -p no:<plugin name>).

This is the first time I understand what this PR really solves 😆 👍

nicoddemus · 2016-06-30T08:58:26Z

Other than my minor comment this looks 👍 to me! 😁

coveralls · 2016-06-30T09:06:09Z

Coverage remained the same at 92.09% when pulling 9e19507 on RonnyPfannschmidt:fix-1618 into ccd395f on pytest-dev:features.

nicoddemus · 2016-07-06T01:03:07Z

testing/test_config.py

+                    pass
+            """,
+        )
+        testdir.inline_run(


This should verify that the test runs OK, so -p could effectively disable the problematic plugin. As it stands, the return of inline_run is just being ignored.

nicoddemus · 2016-07-06T01:17:29Z

I tried your branch with a small example similar to the test, this is what I get:

$ py.test test_f.py
_______________________ ERROR collecting tmp/test_f.py ________________________
..\_pytest\python.py:373: in fget
    return self._obj
E   AttributeError: 'Module' object has no attribute '_obj'

During handling of the above exception, another exception occurred:
plugin.py:1: in <module>
    raise ValueError
E   ValueError
!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!
=========================== 1 error in 0.14 seconds ===========================

(Note I'm not using -p no:plugin).

On master the same command line looks better:

$ py.test test_f.py
=================================== ERRORS ====================================
_______________________ ERROR collecting tmp/test_f.py ________________________
plugin.py:1: in <module>
    raise ValueError
E   ValueError
=========================== 1 error in 0.01 seconds ===========================

Could you take a look at that?

FWIW passing -p no:plugin on your branch works as expected: 👍

$ py.test test_f.py -p no:plugin
============================= test session starts =============================
platform win32 -- Python 3.5.0, pytest-2.10.0.dev1, py-1.4.31, pluggy-0.3.1
rootdir: X:\pytest, inifile: tox.ini
plugins: hypothesis-3.1.0, xdist-1.14
collected 1 items

test_f.py .

========================== 1 passed in 0.01 seconds ===========================

RonnyPfannschmidt · 2016-07-06T04:35:30Z

Can you show the file content?

nicoddemus · 2016-07-06T11:12:51Z

Sorry I don't have them with me right now, but IIRC:

# conftest.py
pytest_plugins = ['plugin']

# plugin.py
raise ValueError

# test_f.py
def test():
    pass

coveralls · 2016-07-06T11:27:24Z

Coverage increased (+0.005%) to 92.095% when pulling 2592c26 on RonnyPfannschmidt:fix-1618 into ccd395f on pytest-dev:features.

nicoddemus · 2016-07-06T15:16:13Z

testing/test_config.py

@@ -352,6 +352,25 @@ def test_inifilename(self, tmpdir):
        assert config.inicfg.get('name') == 'value'
        assert config.inicfg.get('should_not_be_set') is None

+    @pytest.mark.issue1618
+    def test_consider_plugin(self, testdir):
+        pytest.importorskip('xdist')


This test failed in CI, but only on xdist tox environments: I think we should run this in all environments because while I understand this was initially reported and diagnosed with xdist, the problem is general to pytest.

the test itself requires xdist, since the bug is triggered in config.from_dictargs
its simply impossible to test it with normal pytest

the more interesting question is - why do these fail on ci, it works in my local test :/ i wonder where my environment is different

Are you sure? I'm think I tested this fix on a virtual environment without xdist...

Oh yes, I remember, I got the same problem and tested your fix just by running py.test, without passing -n1. If it should be run in both environments, I guess we should parametrize this then.

i am certain that xdist is currently the only thing triggering the behavior

the problem in question is the worker process ignoring the 'no:foo' plugin spec

i am certain that xdist is currently the only thing triggering the behavior

Something is wrong then... just tested on a fresh environment, without any plugins.

master

λ py.test test_foo.py -p no:plugin Traceback (most recent call last): ... File "C:\Users\bruno\addopts-order\src\plugin.py", line 1, in <module> raise ValueError ValueError

RonnyPfannschmidt:fix-1618

λ py.test test_foo.py -p no:plugin ============================= test session starts ============================= platform win32 -- Python 2.7.11, pytest-2.10.0.dev1, py-1.4.31, pluggy-0.3.1 rootdir: C:\Users\bruno\addopts-order\src, inifile: collected 1 items test_foo.py . ========================== 1 passed in 0.01 seconds =========================

That was on Python 2.7.

the more interesting question is - why do these fail on ci, it works in my local test :/ i wonder where my environment is different

Should be res.ret instead of res.res, not sure why that passes for you.

I also executed your test on my new environment, and it passes if I don't have xdist and don't pass -n1 (also changing to res.ret):

import pytest pytest_plugins = ['pytester'] def test_consider_plugin(testdir): # pytest.importorskip('xdist') print (testdir) testdir.makepyfile(conftest=""" pytest_plugins = ['plugin'] """, plugin=""" raise ImportError """, test_foo=""" def test(): pass """, ) res = testdir.inline_run( #'-n1', '-p', 'no:plugin') assert res.ret == 0

λ py.test test_ronny.py ============================= test session starts ============================= platform win32 -- Python 2.7.11, pytest-2.10.0.dev1, py-1.4.31, pluggy-0.3.1 rootdir: C:\Users\bruno\addopts-order\src, inifile: collected 1 items test_ronny.py . ========================== 1 passed in 0.10 seconds ===========================

The same test fails on master as expected:

ImportError: Error importing plugin "plugin"

So I'm guessing there is something weird with your local environment.

nicoddemus · 2016-07-13T22:14:16Z

Any news here @RonnyPfannschmidt?

RonnyPfannschmidt · 2016-07-14T06:59:07Z

@nicoddemus didnt get to touch this one today, hope to get at it in the afternoon

RonnyPfannschmidt · 2016-07-21T08:55:37Z

closing this one, the approach i picked is problematic and configuration construction is "tricky"
in order to approach this in a comprehensible manner, the config object initialization has tobe reworked

RonnyPfannschmidt force-pushed the fix-1618 branch from d1702bd to 5e31ef7 Compare June 25, 2016 16:36

The-Compiler reviewed Jun 25, 2016
View reviewed changes

nicoddemus mentioned this pull request Jun 26, 2016

-p no:pluginname on pytest 2.9.x ignored on xdist slave node #1618

Closed

nicoddemus reviewed Jun 26, 2016
View reviewed changes

RonnyPfannschmidt added 3 commits June 27, 2016 13:18

fix issue pytest-dev#1618 by considering plugin args first

f33da43

additionally prevent unnecessary importation of blocked plugins

add changelog for pytest-dev#1618

8a4b00d

refer the issue pytest-dev#1618 in the test

3c246f5

Fix pytest-dev#1618

RonnyPfannschmidt force-pushed the fix-1618 branch from 5e31ef7 to 3c246f5 Compare June 27, 2016 11:21

RonnyPfannschmidt added 2 commits June 27, 2016 13:30

config tests: use python3 compatible print

64cd2f1

remove duplicate changelog referenece after rebase

5f61109

nicoddemus reviewed Jun 30, 2016
View reviewed changes

extend changelog entry for pytest-dev#1618

9e19507

nicoddemus reviewed Jul 6, 2016
View reviewed changes

assert test execution outcome for pytest-dev#1618 test

2592c26

nicoddemus reviewed Jul 6, 2016
View reviewed changes

nicoddemus mentioned this pull request Jul 6, 2016

Merge from master to features #1705

Merged

RonnyPfannschmidt closed this Jul 21, 2016

RonnyPfannschmidt deleted the fix-1618 branch June 29, 2021 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix issue #1618 by considering plugin args first #1673

fix issue #1618 by considering plugin args first #1673

RonnyPfannschmidt commented Jun 25, 2016

The-Compiler Jun 25, 2016

nicoddemus Jun 26, 2016

nicoddemus Jun 26, 2016

nicoddemus Jun 26, 2016

coveralls commented Jun 27, 2016 •

edited

Loading

coveralls commented Jun 27, 2016 •

edited

Loading

RonnyPfannschmidt commented Jun 30, 2016

nicoddemus Jun 30, 2016

nicoddemus Jul 6, 2016

The-Compiler Jul 6, 2016

nicoddemus commented Jun 30, 2016

coveralls commented Jun 30, 2016 •

edited

Loading

nicoddemus Jul 6, 2016 •

edited

Loading

nicoddemus commented Jul 6, 2016

RonnyPfannschmidt commented Jul 6, 2016

nicoddemus commented Jul 6, 2016

coveralls commented Jul 6, 2016 •

edited

Loading

nicoddemus Jul 6, 2016

RonnyPfannschmidt Jul 6, 2016

RonnyPfannschmidt Jul 6, 2016

nicoddemus Jul 6, 2016

RonnyPfannschmidt Jul 6, 2016

nicoddemus Jul 6, 2016

nicoddemus commented Jul 13, 2016

RonnyPfannschmidt commented Jul 14, 2016

RonnyPfannschmidt commented Jul 21, 2016

fix issue #1618 by considering plugin args first #1673

fix issue #1618 by considering plugin args first #1673

Conversation

RonnyPfannschmidt commented Jun 25, 2016

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

coveralls commented Jun 27, 2016 • edited Loading

coveralls commented Jun 27, 2016 • edited Loading

RonnyPfannschmidt commented Jun 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus commented Jun 30, 2016

coveralls commented Jun 30, 2016 • edited Loading

nicoddemus Jul 6, 2016 • edited Loading

Choose a reason for hiding this comment

nicoddemus commented Jul 6, 2016

RonnyPfannschmidt commented Jul 6, 2016

nicoddemus commented Jul 6, 2016

coveralls commented Jul 6, 2016 • 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

Choose a reason for hiding this comment

nicoddemus commented Jul 13, 2016

RonnyPfannschmidt commented Jul 14, 2016

RonnyPfannschmidt commented Jul 21, 2016

coveralls commented Jun 27, 2016 •

edited

Loading

coveralls commented Jun 27, 2016 •

edited

Loading

coveralls commented Jun 30, 2016 •

edited

Loading

nicoddemus Jul 6, 2016 •

edited

Loading

coveralls commented Jul 6, 2016 •

edited

Loading