Remove UnicodeWarning from pytest warnings

[nicoddemus]

I agree with @dwaynebailey in that it is not pytest's job to issue a warning about this, specially because it might be out of the user's control and the standard library does not prohibit it anyway.

Fix #2463

[coveralls]

Coverage increased (+0.02%) to 92.151% when pulling 0bc4c8a on nicoddemus:remove-unicode-warning into 1863b7c on pytest-dev:master.

[coveralls]

Coverage increased (+0.02%) to 92.151% when pulling 08b5836 on nicoddemus:remove-unicode-warning into 1863b7c on pytest-dev:master.

[RonnyPfannschmidt]

This means code under test will break in the wild

We fix a cpython issue, but no longer tell the user

We either should break completely or warn

[nicoddemus]

This means code under test will break in the wild

What do you mean, could you clarify? After all, code break because of this warning as stated in #2463.

We fix a cpython issue, but no longer tell the user
We either should break completely or warn

Not sure if it is pytest's job to warn users if they are using some other library incorrectly... for example in the original report, reportedly Django uses unicode in their warning messages.

What do you suggest to fix #2463?

[RonnyPfannschmidt]

the fix to #2436 which was committed in #2437 added this warning precidely because we change behaviors under test - if we no longer warn about that, then in some cases we report broken code under test as working

so we either limit the warning printing to strings that are not encodable by ascii, or we let the unicode error happen again in general

[nicoddemus]

TBH I'm not entirely convinced that issuing a warning is the right approach, after all we use safe_str in a bunch of places to the same effect and don't issue warnings on those places.

But nonetheless I implemented the change you requested @RonnyPfannschmidt, now the warning is emitted only if the unicode string contains non-ascii characters.

[RonnyPfannschmidt]

@nicoddemus unlike other places, we modify data that under normal circumstances would be passed trough to the stdlib unchaged, and then would break there - so we hook in between user code and stdlib, and patch out a breakage -

[coveralls]

Coverage decreased (-0.01%) to 92.122% when pulling 9d41eae on nicoddemus:remove-unicode-warning into 5ee9793 on pytest-dev:master.

[RonnyPfannschmidt]

also note that with the change as is, we don't fix #2463 - we just make hitting it less probable

[RonnyPfannschmidt]

@nicoddemus so instead of making this warning invisible, what we really need to do is ensure nobody can make it an error

[nicoddemus]

also note that with the change as is, we don't fix #2463

Noted, I realized it as well, but given your points I don't see an alternative: either we keep the warning in place or we remove it.

so instead of making this warning invisible, what we really need to do is ensure nobody can make it an error

Not sure what you are proposing... why should we prevent people from turning this into an error if they want to? I don't think we should be messing with the warnings filters in minor versions, if that's what you are suggesting...

[RonnyPfannschmidt]

@nicoddemus then we need to document for people what warnings to filter to keep their testsuite working when they want to be pedantic

#2463 creates a problem because pytest has own warnings - we either remove the fix for #2437 alongside the warning or keep the warning

[nicoddemus]

then we need to document for people what warnings to filter to keep their testsuite working when they want to be pedantic

Seems reasonable. How about adding a list of possible warnings generated by pytest itself in the docs? I would like to do that in a separate PR though.

we either remove the fix for #2437 alongside the warning

That doesn't seem to be an option: the warning with unicode message in #2437 was issued by Django, so the user using it has no power to fix that himself. It is not the case of a stubborn user just wanting to silence a warning on their incorrect code, it is a user using an external framework to which he has no control over. Pytest should do its best to not blow up in the face of the user IMO.

[RonnyPfannschmidt]

it is critical to warn on actual non-ascii warnings going trough our fixup code
or we go for removing the code, then broken code rightfully blows up again

what we cant do is hide broken code without warning, especially in the case where real python will blow up because of non-ascii

[nicoddemus]

How about we issue the warning with unicode containing non-ascii characters (like the PR does now) and document a list of warnings that pytest might raise internally?

[RonnyPfannschmidt]

👍

[nicoddemus]

Are you OK then? Could you merge then @RonnyPfannschmidt? Created #2477 for the warnings issue.