From 5436e4299075236023e8875c563a7c74cad9bb2a Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 3 Oct 2018 20:07:59 -0300 Subject: [PATCH] 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 --- changelog/2293.feature.rst | 4 ++ changelog/2293.trivial.rst | 1 + src/_pytest/fixtures.py | 15 ++++--- src/_pytest/mark/__init__.py | 5 --- src/_pytest/mark/structures.py | 3 +- src/_pytest/nodes.py | 4 ++ src/_pytest/python.py | 50 +++++++++++---------- testing/python/fixture.py | 81 ++++++++++++++++------------------ testing/python/metafunc.py | 38 ++++++++-------- testing/test_mark.py | 4 +- testing/test_runner.py | 18 +++++++- 11 files changed, 124 insertions(+), 99 deletions(-) create mode 100644 changelog/2293.feature.rst create mode 100644 changelog/2293.trivial.rst diff --git a/changelog/2293.feature.rst b/changelog/2293.feature.rst new file mode 100644 index 000000000..5e56ba321 --- /dev/null +++ b/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. diff --git a/changelog/2293.trivial.rst b/changelog/2293.trivial.rst new file mode 100644 index 000000000..a11245127 --- /dev/null +++ b/changelog/2293.trivial.rst @@ -0,0 +1 @@ +The internal ``MarkerError`` exception has been removed. diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 0b8b6b044..3dec56a35 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -579,7 +579,7 @@ class FixtureRequest(FuncargnamesCompatAttr): 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 @@ class FixtureRequest(FuncargnamesCompatAttr): 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 @@ class FixtureDef(object): 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) diff --git a/src/_pytest/mark/__init__.py b/src/_pytest/mark/__init__.py index eb25cd4aa..390057428 100644 --- a/src/_pytest/mark/__init__.py +++ b/src/_pytest/mark/__init__.py @@ -24,11 +24,6 @@ __all__ = [ ] -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 `. diff --git a/src/_pytest/mark/structures.py b/src/_pytest/mark/structures.py index ef2757d62..32822c2bb 100644 --- a/src/_pytest/mark/structures.py +++ b/src/_pytest/mark/structures.py @@ -6,6 +6,7 @@ from operator import attrgetter 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 @@ class MarkGenerator(object): 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() diff --git a/src/_pytest/nodes.py b/src/_pytest/nodes.py index 9cd758941..d80895ab5 100644 --- a/src/_pytest/nodes.py +++ b/src/_pytest/nodes.py @@ -9,6 +9,7 @@ import attr 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 @@ class Node(object): 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() diff --git a/src/_pytest/python.py b/src/_pytest/python.py index 258901380..e9d05666f 100644 --- a/src/_pytest/python.py +++ b/src/_pytest/python.py @@ -13,7 +13,6 @@ from textwrap import dedent 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 @@ class FunctionMixin(PyobjMixin): 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 @@ class Metafunc(fixtures.FuncargnamesCompatAttr): 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 @@ class Metafunc(fixtures.FuncargnamesCompatAttr): 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 @@ class Metafunc(fixtures.FuncargnamesCompatAttr): 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 @@ class Metafunc(fixtures.FuncargnamesCompatAttr): :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. diff --git a/testing/python/fixture.py b/testing/python/fixture.py index a9e33b455..f10270326 100644 --- a/testing/python/fixture.py +++ b/testing/python/fixture.py @@ -1217,8 +1217,7 @@ class TestFixtureUsages(object): 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 @@ class TestParameterizedSubRequest(object): ) 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 @@ class TestParameterizedSubRequest(object): ) 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 @@ class TestParameterizedSubRequest(object): ) 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 @@ class TestParameterizedSubRequest(object): 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*", + ] ) diff --git a/testing/python/metafunc.py b/testing/python/metafunc.py index 2172c5e0c..fea59ee98 100644 --- a/testing/python/metafunc.py +++ b/testing/python/metafunc.py @@ -127,10 +127,11 @@ class TestMetafunc(object): pass metafunc = self.Metafunc(func) - try: + with pytest.raises( + pytest.fail.Exception, + match=r"parametrize\(\) call in func got an unexpected scope value 'doggy'", + ): metafunc.parametrize("x", [1], scope="doggy") - except ValueError as ve: - assert "has an unsupported scope value 'doggy'" in str(ve) def test_find_parametrized_scope(self): """unittest for _find_parametrized_scope (#3941)""" @@ -206,16 +207,13 @@ class TestMetafunc(object): metafunc = self.Metafunc(func) - pytest.raises( - ValueError, lambda: metafunc.parametrize("x", [1, 2], ids=["basic"]) - ) + with pytest.raises(pytest.fail.Exception): + metafunc.parametrize("x", [1, 2], ids=["basic"]) - pytest.raises( - ValueError, - lambda: metafunc.parametrize( + with pytest.raises(pytest.fail.Exception): + metafunc.parametrize( ("x", "y"), [("abc", "def"), ("ghi", "jkl")], ids=["one"] - ), - ) + ) @pytest.mark.issue510 def test_parametrize_empty_list(self): @@ -573,7 +571,7 @@ class TestMetafunc(object): pass metafunc = self.Metafunc(func) - with pytest.raises(ValueError): + with pytest.raises(pytest.fail.Exception): metafunc.parametrize("x, y", [("a", "b")], indirect=["x", "z"]) @pytest.mark.issue714 @@ -1189,7 +1187,9 @@ class TestMetafuncFunctional(object): ) result = testdir.runpytest() result.stdout.fnmatch_lines( - ["*ids must be list of strings, found: 2 (type: int)*"] + [ + "*In test_ids_numbers: ids must be list of strings, found: 2 (type: *'int'>)*" + ] ) def test_parametrize_with_identical_ids_get_unique_names(self, testdir): @@ -1326,13 +1326,13 @@ class TestMetafuncFunctional(object): attr ) ) - reprec = testdir.inline_run("--collectonly") - failures = reprec.getfailures() - assert len(failures) == 1 - expectederror = "MarkerError: test_foo has '{}', spelling should be 'parametrize'".format( - attr + result = testdir.runpytest("--collectonly") + result.stdout.fnmatch_lines( + [ + "test_foo has '{}' mark, spelling should be 'parametrize'".format(attr), + "*1 error in*", + ] ) - assert expectederror in failures[0].longrepr.reprcrash.message class TestMetafuncFunctionalAuto(object): diff --git a/testing/test_mark.py b/testing/test_mark.py index 3e5c86624..48a680297 100644 --- a/testing/test_mark.py +++ b/testing/test_mark.py @@ -247,7 +247,7 @@ def test_marker_without_description(testdir): ) ftdir = testdir.mkdir("ft1_dummy") testdir.tmpdir.join("conftest.py").move(ftdir.join("conftest.py")) - rec = testdir.runpytest_subprocess("--strict") + rec = testdir.runpytest("--strict") rec.assert_outcomes() @@ -302,7 +302,7 @@ def test_strict_prohibits_unregistered_markers(testdir): ) result = testdir.runpytest("--strict") assert result.ret != 0 - result.stdout.fnmatch_lines(["*unregisteredmark*not*registered*"]) + result.stdout.fnmatch_lines(["'unregisteredmark' not a registered marker"]) @pytest.mark.parametrize( diff --git a/testing/test_runner.py b/testing/test_runner.py index 741180692..b9538cfad 100644 --- a/testing/test_runner.py +++ b/testing/test_runner.py @@ -570,7 +570,8 @@ def test_pytest_exit_msg(testdir): result.stderr.fnmatch_lines(["Exit: oh noes"]) -def test_pytest_fail_notrace(testdir): +def test_pytest_fail_notrace_runtest(testdir): + """Test pytest.fail(..., pytrace=False) does not show tracebacks during test run.""" testdir.makepyfile( """ import pytest @@ -585,6 +586,21 @@ def test_pytest_fail_notrace(testdir): assert "def teardown_function" not in result.stdout.str() +def test_pytest_fail_notrace_collection(testdir): + """Test pytest.fail(..., pytrace=False) does not show tracebacks during collection.""" + testdir.makepyfile( + """ + import pytest + def some_internal_function(): + pytest.fail("hello", pytrace=False) + some_internal_function() + """ + ) + result = testdir.runpytest() + result.stdout.fnmatch_lines(["hello"]) + assert "def some_internal_function()" not in result.stdout.str() + + @pytest.mark.parametrize("str_prefix", ["u", ""]) def test_pytest_fail_notrace_non_ascii(testdir, str_prefix): """Fix pytest.fail with pytrace=False with non-ascii characters (#1178).