Fix parametrize(... ids=<function>) when the function returns non-strings
#4740
Conversation
| except Exception as e: | ||
| # See issue https://github.com/pytest-dev/pytest/issues/2169 | ||
| msg = "{}: error raised while trying to determine id of parameter '{}' at position {}\n" | ||
| msg = msg.format(item.nodeid, argname, idx) | ||
| # we only append the exception type and message because on Python 2 reraise does nothing | ||
| msg += " {}: {}\n".format(type(e).__name__, e) | ||
| six.raise_from(ValueError(msg), e) | ||
| if s: | ||
| return ascii_escaped(s) |
There was a problem hiding this comment.
this is a behavior change, the hook now can override the given id
additionally i noted we font factor in the ascii_escape of the id from the hook
i propose factoring out the escaping logic to a function that gets a fallback result and using that to wrap around the return value in the different cases
There was a problem hiding this comment.
headscratch I'm not seeing the behaviour change -- can you shed some light? The only behaviour I intended to change is crash -> not crash (and restore what happened in pytest 3.0.7)
There was a problem hiding this comment.
by moving the idfn handling from returning before the pytest_make_parametrize_id to after invoking it,
the behavior is accidentally changed, as now the hook can override the result of the idfn, while before it couldn't
There was a problem hiding this comment.
That sounds like an improvement, no?
There was a problem hiding this comment.
as done here its possible that this inadvertely changes ids in a testsuite, so its a change for a major release
There was a problem hiding this comment.
seems broken though that in this weird special case we don't apply the hook
There was a problem hiding this comment.
It is broken, its just that bugfixing is a breaking behavioral change
There was a problem hiding this comment.
how if fixing any bug not a behavioural change 🤣
There was a problem hiding this comment.
@RonnyPfannschmidt better now?
There was a problem hiding this comment.
it seems much better, my gut is not 100% happy with the solution we have tho,
i'd really like to note down to change the behavior for pytest 6.0 (avoiding it in 5.0 for the management of the pytohn support drop)
| except Exception as e: | ||
| # See issue https://github.com/pytest-dev/pytest/issues/2169 | ||
| msg = "{}: error raised while trying to determine id of parameter '{}' at position {}\n" | ||
| msg = msg.format(item.nodeid, argname, idx) | ||
| # we only append the exception type and message because on Python 2 reraise does nothing | ||
| msg += " {}: {}\n".format(type(e).__name__, e) | ||
| six.raise_from(ValueError(msg), e) | ||
| if s: | ||
| return ascii_escaped(s) |
There was a problem hiding this comment.
it seems much better, my gut is not 100% happy with the solution we have tho,
i'd really like to note down to change the behavior for pytest 6.0 (avoiding it in 5.0 for the management of the pytohn support drop)
|
@asottile would you like to create the followup issue, otherwise i will |
I think you'll do it better justice than I will 👍 |
|
thanks, i'm on it then |
Resolves #4739