From d81f23009bda9e31bd8ccc4c8300fcbaad56fe5c Mon Sep 17 00:00:00 2001 From: Omar Kohl Date: Sun, 10 Apr 2016 19:57:45 +0200 Subject: [PATCH 1/2] Raise CollectError if pytest.skip() is called during collection pytest.skip() must not be used at module level because it can easily be misunderstood and used as a decorator instead of pytest.mark.skip, causing the whole module to be skipped instead of just the test being decorated. This is unexpected for users used to the @unittest.skip decorator and therefore it is best to bail out with a clean error when it happens. The pytest equivalent of @unittest.skip is @pytest.mark.skip . Adapt existing tests that were actually relying on this behaviour and add a test that explicitly test that collection fails. fix #607 --- _pytest/python.py | 6 ++++ testing/test_junitxml.py | 59 +++++++++++++++++++++++++++------------ testing/test_resultlog.py | 9 ------ testing/test_runner.py | 12 -------- testing/test_session.py | 8 +----- testing/test_skipping.py | 22 +++++++++++---- testing/test_terminal.py | 3 +- 7 files changed, 66 insertions(+), 53 deletions(-) diff --git a/_pytest/python.py b/_pytest/python.py index 1f65eee59f8..0520ecef0c7 100644 --- a/_pytest/python.py +++ b/_pytest/python.py @@ -656,6 +656,12 @@ def _importtestmodule(self): "Make sure your test modules/packages have valid Python names." % (self.fspath, exc or exc_class) ) + except _pytest.runner.Skipped: + raise self.CollectError( + "Using @pytest.skip outside a test (e.g. as a test function " + "decorator) is not allowed. Use @pytest.mark.skip or " + "@pytest.mark.skipif instead." + ) #print "imported test module", mod self.config.pluginmanager.consider_module(mod) return mod diff --git a/testing/test_junitxml.py b/testing/test_junitxml.py index fde003e315b..8291ea148b3 100644 --- a/testing/test_junitxml.py +++ b/testing/test_junitxml.py @@ -1,7 +1,6 @@ # -*- coding: utf-8 -*- from xml.dom import minidom -from _pytest.main import EXIT_NOTESTSCOLLECTED import py import sys import os @@ -158,6 +157,47 @@ def test_skip(): snode = tnode.find_first_by_tag("skipped") snode.assert_attr(type="pytest.skip", message="hello23", ) + def test_mark_skip_contains_name_reason(self, testdir): + testdir.makepyfile(""" + import pytest + @pytest.mark.skip(reason="hello24") + def test_skip(): + assert True + """) + result, dom = runandparse(testdir) + assert result.ret == 0 + node = dom.find_first_by_tag("testsuite") + node.assert_attr(skips=1) + tnode = node.find_first_by_tag("testcase") + tnode.assert_attr( + file="test_mark_skip_contains_name_reason.py", + line="1", + classname="test_mark_skip_contains_name_reason", + name="test_skip") + snode = tnode.find_first_by_tag("skipped") + snode.assert_attr(type="pytest.skip", message="hello24", ) + + def test_mark_skipif_contains_name_reason(self, testdir): + testdir.makepyfile(""" + import pytest + GLOBAL_CONDITION = True + @pytest.mark.skipif(GLOBAL_CONDITION, reason="hello25") + def test_skip(): + assert True + """) + result, dom = runandparse(testdir) + assert result.ret == 0 + node = dom.find_first_by_tag("testsuite") + node.assert_attr(skips=1) + tnode = node.find_first_by_tag("testcase") + tnode.assert_attr( + file="test_mark_skipif_contains_name_reason.py", + line="2", + classname="test_mark_skipif_contains_name_reason", + name="test_skip") + snode = tnode.find_first_by_tag("skipped") + snode.assert_attr(type="pytest.skip", message="hello25", ) + def test_classname_instance(self, testdir): testdir.makepyfile(""" class TestClass: @@ -351,23 +391,6 @@ def test_collect_error(self, testdir): fnode.assert_attr(message="collection failure") assert "SyntaxError" in fnode.toxml() - def test_collect_skipped(self, testdir): - testdir.makepyfile("import pytest; pytest.skip('xyz')") - result, dom = runandparse(testdir) - assert result.ret == EXIT_NOTESTSCOLLECTED - node = dom.find_first_by_tag("testsuite") - node.assert_attr(skips=1, tests=1) - tnode = node.find_first_by_tag("testcase") - tnode.assert_attr( - file="test_collect_skipped.py", - name="test_collect_skipped") - - # pytest doesn't give us a line here. - assert tnode["line"] is None - - fnode = tnode.find_first_by_tag("skipped") - fnode.assert_attr(message="collection skipped") - def test_unicode(self, testdir): value = 'hx\xc4\x85\xc4\x87\n' testdir.makepyfile(""" diff --git a/testing/test_resultlog.py b/testing/test_resultlog.py index 373d1921381..e2d4fc26367 100644 --- a/testing/test_resultlog.py +++ b/testing/test_resultlog.py @@ -83,19 +83,10 @@ def getresultlog(self, testdir, arg): def test_collection_report(self, testdir): ok = testdir.makepyfile(test_collection_ok="") - skip = testdir.makepyfile(test_collection_skip= - "import pytest ; pytest.skip('hello')") fail = testdir.makepyfile(test_collection_fail="XXX") lines = self.getresultlog(testdir, ok) assert not lines - lines = self.getresultlog(testdir, skip) - assert len(lines) == 2 - assert lines[0].startswith("S ") - assert lines[0].endswith("test_collection_skip.py") - assert lines[1].startswith(" ") - assert lines[1].endswith("test_collection_skip.py:1: Skipped: hello") - lines = self.getresultlog(testdir, fail) assert lines assert lines[0].startswith("F ") diff --git a/testing/test_runner.py b/testing/test_runner.py index 377801132c7..7db809b24cd 100644 --- a/testing/test_runner.py +++ b/testing/test_runner.py @@ -365,18 +365,6 @@ class TestClass: assert res[0].name == "test_func1" assert res[1].name == "TestClass" - def test_skip_at_module_scope(self, testdir): - col = testdir.getmodulecol(""" - import pytest - pytest.skip("hello") - def test_func(): - pass - """) - rep = main.collect_one_node(col) - assert not rep.failed - assert not rep.passed - assert rep.skipped - reporttypes = [ runner.BaseReport, diff --git a/testing/test_session.py b/testing/test_session.py index 8c9386a12e7..e3b9f0fcccf 100644 --- a/testing/test_session.py +++ b/testing/test_session.py @@ -174,10 +174,6 @@ def test_method_one(self): class TestY(TestX): pass """, - test_two=""" - import pytest - pytest.skip('xxx') - """, test_three="xxxdsadsadsadsa", __init__="" ) @@ -189,11 +185,9 @@ class TestY(TestX): started = reprec.getcalls("pytest_collectstart") finished = reprec.getreports("pytest_collectreport") assert len(started) == len(finished) - assert len(started) == 8 # XXX extra TopCollector + assert len(started) == 7 # XXX extra TopCollector colfail = [x for x in finished if x.failed] - colskipped = [x for x in finished if x.skipped] assert len(colfail) == 1 - assert len(colskipped) == 1 def test_minus_x_import_error(self, testdir): testdir.makepyfile(__init__="") diff --git a/testing/test_skipping.py b/testing/test_skipping.py index 2bfb6a8dc58..f8e85d446d8 100644 --- a/testing/test_skipping.py +++ b/testing/test_skipping.py @@ -682,10 +682,6 @@ class TestClass: def test_method(self): doskip() """, - test_two = """ - from conftest import doskip - doskip() - """, conftest = """ import pytest def doskip(): @@ -694,7 +690,7 @@ def doskip(): ) result = testdir.runpytest('--report=skipped') result.stdout.fnmatch_lines([ - "*SKIP*3*conftest.py:3: test", + "*SKIP*2*conftest.py:3: test", ]) assert result.ret == 0 @@ -941,3 +937,19 @@ def pytest_collect_file(path, parent): assert not failed xfailed = [r for r in skipped if hasattr(r, 'wasxfail')] assert xfailed + + +def test_module_level_skip_error(testdir): + """ + Verify that using pytest.skip at module level causes a collection error + """ + testdir.makepyfile(""" + import pytest + @pytest.skip + def test_func(): + assert True + """) + result = testdir.runpytest() + result.stdout.fnmatch_lines( + "*Using @pytest.skip outside a test * is not allowed*" + ) diff --git a/testing/test_terminal.py b/testing/test_terminal.py index 909450f5545..589881c2375 100644 --- a/testing/test_terminal.py +++ b/testing/test_terminal.py @@ -223,8 +223,7 @@ def test_collectonly_skipped_module(self, testdir): """) result = testdir.runpytest("--collect-only", "-rs") result.stdout.fnmatch_lines([ - "SKIP*hello*", - "*1 skip*", + "*ERROR collecting*", ]) def test_collectonly_failed_module(self, testdir): From b3615ac29db523ae4071b7cd4b8ad35696f43b54 Mon Sep 17 00:00:00 2001 From: Omar Kohl Date: Tue, 19 Apr 2016 20:37:54 +0200 Subject: [PATCH 2/2] Update CHANGELOG with #607 and add version 3.0.0.dev1 --- CHANGELOG.rst | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index d95d66384ed..7488694adcd 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -1,3 +1,24 @@ +3.0.0.dev1 +========== + +**Changes** + +* + +* + +* + +* Fix (`#607`_): pytest.skip() is no longer allowed at module level to + prevent misleading use as test function decorator. When used at a module + level an error will be raised during collection. + Thanks `@omarkohl`_ for the complete PR (`#1519`_). + +* + +.. _#607: https://github.com/pytest-dev/pytest/issues/607 +.. _#1519: https://github.com/pytest-dev/pytest/pull/1519 + 2.10.0.dev1 ===========