No longer silently ignore errors in parametrize callable ids by fogo · Pull Request #2167 · pytest-dev/pytest

fogo · 2016-12-30T17:48:16Z

I had recently an issue with a complex parametrized test with a callable ids. The ids function was wrong but it took me sometime to notice as pytest was silently swallowing any errors when it was called.

I changed the code necessary so this won't happen anymore and modified/created some tests.

One more thing though, a question for you, guys: I tried to do some kind of raise...from with a better exception with fixture name but the meta-function information doesn't propagate up to ids function (unlike the config object, which happens to be available there, though it isn't mandatory). If the meta-function got there it'd probably be possible to be way clearer about error, what do you think about this?

nicoddemus

Also, please rebase your PR on the features branch. This is a behavior change (for the better IMO), but even so should not be made in a bug-fix release.

nicoddemus · 2016-12-30T17:56:01Z

@@ -300,14 +300,35 @@ def test_idmaker_idfn_exception(self):
        def ids(val):
            raise Exception("bad code")


Probably better to raise a more unique exception here, like ValueError("error on ids") and catch that in pytest.raises below, also checking that is the same message.

Otherwise in the future someone might refactor this and then another exception is raised and this test won't catch it.

coveralls · 2016-12-30T17:58:51Z

Coverage decreased (-0.002%) to 92.814% when pulling 51912f9 on fogo:parametrize-ids-silent-failure into 7592c5b on pytest-dev:master.

coveralls · 2016-12-30T18:07:11Z

Coverage decreased (-0.002%) to 92.814% when pulling 51912f9 on fogo:parametrize-ids-silent-failure into 7592c5b on pytest-dev:master.

RonnyPfannschmidt · 2016-12-31T08:13:21Z

As far as I can tell this change as is warrants a 4.0 release as it is breaking things

nicoddemus · 2017-01-01T22:17:09Z

As far as I can tell this change as is warrants a 4.0 release as it is breaking things

I don't agree @RonnyPfannschmidt... it is a small change, we don't need to make a 4.0 release just because of it. If somebody is relying on this (which I doubt) it is just a matter of adding a try/except over their id function. I think releasing it in 3.1 is fine.

RonnyPfannschmidt · 2017-01-02T07:30:40Z

This is a matter oft principle, any breaking change we known of gets a major release or we make it non breaking

fogo · 2017-01-02T12:44:21Z

I don't really mind myself in which version it is released (I doubt many people are depending on this behavior I understand why you wouldn't want to break compatibility), I just think that current behavior is surprising in a harmful way.

In my case I only noticed it when I tried to learn which argument had crashed in a tests log, only to discover that ids wasn't really working,

nicoddemus · 2017-01-02T12:54:48Z

This is a matter oft principle, any breaking change we known of gets a major release or we make it non breaking

Hmm OK. What if we make this an opt-in option then? For example: ignore_parametrize_id_function option in the ini file, defaulting to False? This way we also have a way to change the default to True in 4.0, also providing a clear way for users to fallback to the previous behavior if they want to.

coveralls · 2017-01-02T13:03:53Z

Coverage decreased (-0.002%) to 92.687% when pulling cc38ce0 on fogo:parametrize-ids-silent-failure into 964ccb9 on pytest-dev:features.

RonnyPfannschmidt · 2017-01-02T13:21:02Z

i would propose to issue a UserWarning or a pytest warning until pytest 4.0 where it should turn into a real exception

at the location where the try/catch diaper was, it is possible to consider the config (if it is not None)

we should have a discussion if this should trigger on py.test --strict instead of an own option

nicoddemus · 2017-01-02T14:43:16Z

i would propose to issue a UserWarning or a pytest warning until pytest 4.0 where it should turn into a real exception

Oh I like this idea. Simpler and we don't need a configuration variable. We can then just remove the try/except in 4.0.

@fogo what do you think?

fogo · 2017-01-02T14:54:34Z

I've implemented your suggestions as a fixup commit, to be easier to compare with what existed. See what you think about them and if they are ok I'll just rebase later.

coveralls · 2017-01-02T15:04:27Z

Coverage increased (+0.01%) to 92.702% when pulling b363f51 on fogo:parametrize-ids-silent-failure into 964ccb9 on pytest-dev:features.

nicoddemus

Apart from my comment about mentioning that this will turn into an error on pytest 4, LGTM!

nicoddemus · 2017-01-02T18:16:58Z

+            s = idfn(val)
+        except Exception:
+            import warnings
+            warnings.warn("Raised while trying to determine id of parameter %s at position %d" % (argname, idx))


I think this will fit well when we integrate pytest-warnings into 3.1. 😁

Could you please add that this will turn into an error on pytest-4.0, something like:

msg = "Raised while trying to determine id of parameter %s at position %d" % (argname, idx) msg += '\nNote that this will turn into an error in pytest-4.0.' warnings.warn(msg)

Don't you guys think it'd be better to create an issue for this? Otherwise it may be forgotten when version 4.0 is finally developed.

Sounds good!

coveralls · 2017-01-02T19:33:51Z

Coverage increased (+0.01%) to 92.704% when pulling a9193a1 on fogo:parametrize-ids-silent-failure into 964ccb9 on pytest-dev:features.

RonnyPfannschmidt · 2017-01-03T08:18:47Z

@fogo thanks for the fabulous work and for sticking up with the nitpicking

fogo · 2017-01-03T09:59:10Z

Thanks to you too, guys. I appreciate very much all the care you all take to keep pytest on track.

vreuter · 2017-02-22T16:11:14Z

This will be nice!

fogo force-pushed the parametrize-ids-silent-failure branch from b699476 to 51912f9 Compare December 30, 2016 17:48

nicoddemus requested changes Dec 30, 2016

View reviewed changes

fogo force-pushed the parametrize-ids-silent-failure branch from 51912f9 to cc38ce0 Compare January 2, 2017 12:53

fogo changed the base branch from master to features January 2, 2017 12:55

nicoddemus requested changes Jan 2, 2017

View reviewed changes

fogo mentioned this pull request Jan 2, 2017

Don't let callable ids in parametrized tests fail silently #2169

Closed

fogo force-pushed the parametrize-ids-silent-failure branch from b363f51 to a9193a1 Compare January 2, 2017 19:23

No longer silently ignore errors in parametrize callable ids

a9193a1

nicoddemus approved these changes Jan 2, 2017

View reviewed changes

RonnyPfannschmidt merged commit 8f14501 into pytest-dev:features Jan 3, 2017

		@@ -300,14 +300,35 @@ def test_idmaker_idfn_exception(self):
		def ids(val):
		raise Exception("bad code")

Uh oh!

Conversation

fogo commented Dec 30, 2016

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

nicoddemus Dec 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fogo Jan 2, 2017

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 30, 2016

Uh oh!

coveralls commented Dec 30, 2016

Uh oh!

RonnyPfannschmidt commented Dec 31, 2016

Uh oh!

nicoddemus commented Jan 1, 2017

Uh oh!

RonnyPfannschmidt commented Jan 2, 2017

Uh oh!

fogo commented Jan 2, 2017

Uh oh!

nicoddemus commented Jan 2, 2017

Uh oh!

coveralls commented Jan 2, 2017

Uh oh!

RonnyPfannschmidt commented Jan 2, 2017

Uh oh!

nicoddemus commented Jan 2, 2017

Uh oh!

fogo commented Jan 2, 2017

Uh oh!

coveralls commented Jan 2, 2017

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

nicoddemus Jan 2, 2017

Choose a reason for hiding this comment

Uh oh!

fogo Jan 2, 2017

Choose a reason for hiding this comment

Uh oh!

nicoddemus Jan 2, 2017

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 2, 2017

Uh oh!

RonnyPfannschmidt commented Jan 3, 2017

Uh oh!

fogo commented Jan 3, 2017

Uh oh!

vreuter commented Feb 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nicoddemus Dec 30, 2016 •

edited

Loading