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

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

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("""

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

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,

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__="")

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*"
+    )

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