Use pytest.fail(..., pytrace=False) when treating user errors

This prevents an enormous and often useless stack trace from showing
to end users.

Fix #3867
Fix #2293

Author: nicoddemus

changelog/2293.feature.rst

@@ -0,0 +1,4 @@
+Improve usage errors messages by hiding internal details which can be distracting and noisy.
+
+This has the side effect that some error conditions that previously raised generic errors (such as
+``ValueError`` for unregistered marks) are now raising ``Failed`` exceptions.

changelog/2293.trivial.rst

@@ -0,0 +1 @@
+The internal ``MarkerError`` exception has been removed.

src/_pytest/fixtures.py

@@ -579,7 +579,7 @@ def _compute_fixture_value(self, fixturedef):
                     nodeid=funcitem.nodeid,
                     typename=type(funcitem).__name__,
                 )
-                fail(msg)
+                fail(msg, pytrace=False)
             if has_params:
                 frame = inspect.stack()[3]
                 frameinfo = inspect.getframeinfo(frame[0])
@@ -600,7 +600,7 @@ def _compute_fixture_value(self, fixturedef):
                         source_lineno,
                     )
                 )
-                fail(msg)
+                fail(msg, pytrace=False)
         else:
             # indices might not be set if old-style metafunc.addcall() was used
             param_index = funcitem.callspec.indices.get(argname, 0)
@@ -718,10 +718,11 @@ def scope2index(scope, descr, where=None):
     try:
         return scopes.index(scope)
     except ValueError:
-        raise ValueError(
-            "{} {}has an unsupported scope value '{}'".format(
+        fail(
+            "{} {}got an unexpected scope value '{}'".format(
                 descr, "from {} ".format(where) if where else "", scope
-            )
+            ),
+            pytrace=False,
         )
 
 
@@ -854,7 +855,9 @@ def __init__(
         self.argname = argname
         self.scope = scope
         self.scopenum = scope2index(
-            scope or "function", descr="fixture {}".format(func.__name__), where=baseid
+            scope or "function",
+            descr="Fixture '{}'".format(func.__name__),
+            where=baseid,
         )
         self.params = params
         self.argnames = getfuncargnames(func, is_method=unittest)

src/_pytest/mark/__init__.py

@@ -24,11 +24,6 @@
 ]
 
 
-class MarkerError(Exception):
-
-    """Error in use of a pytest marker/attribute."""
-
-
 def param(*values, **kw):
     """Specify a parameter in `pytest.mark.parametrize`_ calls or
     :ref:`parametrized fixtures <fixture-parametrize-marks>`.

src/_pytest/mark/structures.py

@@ -6,6 +6,7 @@
 
 import attr
 
+from _pytest.outcomes import fail
 from ..deprecated import MARK_PARAMETERSET_UNPACKING, MARK_INFO_ATTRIBUTE
 from ..compat import NOTSET, getfslineno, MappingMixin
 from six.moves import map
@@ -393,7 +394,7 @@ def _check(self, name):
             x = marker.split("(", 1)[0]
             values.add(x)
         if name not in self._markers:
-            raise AttributeError("%r not a registered marker" % (name,))
+            fail("{!r} not a registered marker".format(name), pytrace=False)
 
 
 MARK_GEN = MarkGenerator()

src/_pytest/nodes.py

@@ -9,6 +9,7 @@
 import _pytest
 import _pytest._code
 from _pytest.compat import getfslineno
+from _pytest.outcomes import fail
 
 from _pytest.mark.structures import NodeKeywords, MarkInfo
 
@@ -346,6 +347,9 @@ def _prunetraceback(self, excinfo):
         pass
 
     def _repr_failure_py(self, excinfo, style=None):
+        if excinfo.errisinstance(fail.Exception):
+            if not excinfo.value.pytrace:
+                return six.text_type(excinfo.value)
         fm = self.session._fixturemanager
         if excinfo.errisinstance(fm.FixtureLookupError):
             return excinfo.value.formatrepr()

src/_pytest/python.py

@@ -13,7 +13,6 @@
 import py
 import six
 from _pytest.main import FSHookProxy
-from _pytest.mark import MarkerError
 from _pytest.config import hookimpl
 
 import _pytest
@@ -159,8 +158,8 @@ def pytest_generate_tests(metafunc):
     alt_spellings = ["parameterize", "parametrise", "parameterise"]
     for attr in alt_spellings:
         if hasattr(metafunc.function, attr):
-            msg = "{0} has '{1}', spelling should be 'parametrize'"
-            raise MarkerError(msg.format(metafunc.function.__name__, attr))
+            msg = "{0} has '{1}' mark, spelling should be 'parametrize'"
+            fail(msg.format(metafunc.function.__name__, attr), pytrace=False)
     for marker in metafunc.definition.iter_markers(name="parametrize"):
         metafunc.parametrize(*marker.args, **marker.kwargs)
 
@@ -760,12 +759,6 @@ def _prunetraceback(self, excinfo):
                     for entry in excinfo.traceback[1:-1]:
                         entry.set_repr_style("short")
 
-    def _repr_failure_py(self, excinfo, style="long"):
-        if excinfo.errisinstance(fail.Exception):
-            if not excinfo.value.pytrace:
-                return six.text_type(excinfo.value)
-        return super(FunctionMixin, self)._repr_failure_py(excinfo, style=style)
-
     def repr_failure(self, excinfo, outerr=None):
         assert outerr is None, "XXX outerr usage is deprecated"
         style = self.config.option.tbstyle
@@ -987,7 +980,9 @@ def parametrize(self, argnames, argvalues, indirect=False, ids=None, scope=None)
 
         ids = self._resolve_arg_ids(argnames, ids, parameters, item=self.definition)
 
-        scopenum = scope2index(scope, descr="call to {}".format(self.parametrize))
+        scopenum = scope2index(
+            scope, descr="parametrize() call in {}".format(self.function.__name__)
+        )
 
         # create the new calls: if we are parametrize() multiple times (by applying the decorator
         # more than once) then we accumulate those calls generating the cartesian product
@@ -1026,15 +1021,16 @@ def _resolve_arg_ids(self, argnames, ids, parameters, item):
             idfn = ids
             ids = None
         if ids:
+            func_name = self.function.__name__
             if len(ids) != len(parameters):
-                raise ValueError(
-                    "%d tests specified with %d ids" % (len(parameters), len(ids))
-                )
+                msg = "In {}: {} parameter sets specified, with different number of ids: {}"
+                fail(msg.format(func_name, len(parameters), len(ids)), pytrace=False)
             for id_value in ids:
                 if id_value is not None and not isinstance(id_value, six.string_types):
-                    msg = "ids must be list of strings, found: %s (type: %s)"
-                    raise ValueError(
-                        msg % (saferepr(id_value), type(id_value).__name__)
+                    msg = "In {}: ids must be list of strings, found: {} (type: {!r})"
+                    fail(
+                        msg.format(func_name, saferepr(id_value), type(id_value)),
+                        pytrace=False,
                     )
         ids = idmaker(argnames, parameters, idfn, ids, self.config, item=item)
         return ids
@@ -1059,9 +1055,11 @@ def _resolve_arg_value_types(self, argnames, indirect):
             valtypes = dict.fromkeys(argnames, "funcargs")
             for arg in indirect:
                 if arg not in argnames:
-                    raise ValueError(
-                        "indirect given to %r: fixture %r doesn't exist"
-                        % (self.function, arg)
+                    fail(
+                        "In {}: indirect fixture '{}' doesn't exist".format(
+                            self.function.__name__, arg
+                        ),
+                        pytrace=False,
                     )
                 valtypes[arg] = "params"
         return valtypes
@@ -1075,19 +1073,25 @@ def _validate_if_using_arg_names(self, argnames, indirect):
         :raise ValueError: if validation fails.
         """
         default_arg_names = set(get_default_arg_names(self.function))
+        func_name = self.function.__name__
         for arg in argnames:
             if arg not in self.fixturenames:
                 if arg in default_arg_names:
-                    raise ValueError(
-                        "%r already takes an argument %r with a default value"
-                        % (self.function, arg)
+                    fail(
+                        "In {}: function already takes an argument '{}' with a default value".format(
+                            func_name, arg
+                        ),
+                        pytrace=False,
                     )
                 else:
                     if isinstance(indirect, (tuple, list)):
                         name = "fixture" if arg in indirect else "argument"
                     else:
                         name = "fixture" if indirect else "argument"
-                    raise ValueError("%r uses no %s %r" % (self.function, name, arg))
+                    fail(
+                        "In {}: function uses no {} '{}'".format(func_name, name, arg),
+                        pytrace=False,
+                    )
 
     def addcall(self, funcargs=None, id=NOTSET, param=NOTSET):
         """ Add a new call to the underlying test function during the collection phase of a test run.

testing/python/fixture.py

@@ -1217,8 +1217,7 @@ def test_nothing(badscope):
         result = testdir.runpytest_inprocess()
         result.stdout.fnmatch_lines(
             (
-                "*ValueError: fixture badscope from test_invalid_scope.py has an unsupported"
-                " scope value 'functions'"
+                "*Fixture 'badscope' from test_invalid_scope.py got an unexpected scope value 'functions'"
             )
         )
 
@@ -3607,16 +3606,15 @@ def test_foo(request, get_named_fixture):
         )
         result = testdir.runpytest()
         result.stdout.fnmatch_lines(
-            """
-            E*Failed: The requested fixture has no parameter defined for test:
-            E*    test_call_from_fixture.py::test_foo
-            E*
-            E*Requested fixture 'fix_with_param' defined in:
-            E*test_call_from_fixture.py:4
-            E*Requested here:
-            E*test_call_from_fixture.py:9
-            *1 error*
-            """
+            [
+                "The requested fixture has no parameter defined for test:",
+                "    test_call_from_fixture.py::test_foo",
+                "Requested fixture 'fix_with_param' defined in:",
+                "test_call_from_fixture.py:4",
+                "Requested here:",
+                "test_call_from_fixture.py:9",
+                "*1 error in*",
+            ]
         )
 
     def test_call_from_test(self, testdir):
@@ -3634,16 +3632,15 @@ def test_foo(request):
         )
         result = testdir.runpytest()
         result.stdout.fnmatch_lines(
-            """
-            E*Failed: The requested fixture has no parameter defined for test:
-            E*    test_call_from_test.py::test_foo
-            E*
-            E*Requested fixture 'fix_with_param' defined in:
-            E*test_call_from_test.py:4
-            E*Requested here:
-            E*test_call_from_test.py:8
-            *1 failed*
-            """
+            [
+                "The requested fixture has no parameter defined for test:",
+                "    test_call_from_test.py::test_foo",
+                "Requested fixture 'fix_with_param' defined in:",
+                "test_call_from_test.py:4",
+                "Requested here:",
+                "test_call_from_test.py:8",
+                "*1 failed*",
+            ]
         )
 
     def test_external_fixture(self, testdir):
@@ -3665,16 +3662,16 @@ def test_foo(request):
         )
         result = testdir.runpytest()
         result.stdout.fnmatch_lines(
-            """
-            E*Failed: The requested fixture has no parameter defined for test:
-            E*    test_external_fixture.py::test_foo
-            E*
-            E*Requested fixture 'fix_with_param' defined in:
-            E*conftest.py:4
-            E*Requested here:
-            E*test_external_fixture.py:2
-            *1 failed*
-            """
+            [
+                "The requested fixture has no parameter defined for test:",
+                "    test_external_fixture.py::test_foo",
+                "",
+                "Requested fixture 'fix_with_param' defined in:",
+                "conftest.py:4",
+                "Requested here:",
+                "test_external_fixture.py:2",
+                "*1 failed*",
+            ]
         )
 
     def test_non_relative_path(self, testdir):
@@ -3709,16 +3706,16 @@ def test_foo(request):
         testdir.syspathinsert(fixdir)
         result = testdir.runpytest()
         result.stdout.fnmatch_lines(
-            """
-            E*Failed: The requested fixture has no parameter defined for test:
-            E*    test_foos.py::test_foo
-            E*
-            E*Requested fixture 'fix_with_param' defined in:
-            E*fix.py:4
-            E*Requested here:
-            E*test_foos.py:4
-            *1 failed*
-            """
+            [
+                "The requested fixture has no parameter defined for test:",
+                "    test_foos.py::test_foo",
+                "",
+                "Requested fixture 'fix_with_param' defined in:",
+                "*fix.py:4",
+                "Requested here:",
+                "test_foos.py:4",
+                "*1 failed*",
+            ]
         )