Escape both bytes and unicode strings for "ids" in Metafunc.parametrize

[ceridwen]

This is the new version of #1463 based on the features branch.

[nicoddemus]

@ceridwen could you please solve the conflict? Sorry about that, it must have been another merge that happened earlier.

[ceridwen]

I solved the conflict. I haven't changed the tests yet. Is my proposed resolution acceptable? I'm planning to repurpose test_unicode_idval_python to check for escaping.

Should I change any of the other nearby tests to check for escaping? Should I write a Hypothesis test for this code? I'm willing to use my own judgment here, but I don't want to trip up anyone else in doing so. If you want me to decide, tell me.

[nicoddemus]

@ceridwen thanks!

I tried your _escape_strings function with this code:

print(_escape_strings(b'bytes-ascii'))
print(_escape_strings(u'unicode-ascii'))
print(_escape_strings(u'unicode-non-ascii:☺'))
print(_escape_strings(b'\xe7-bytes'))

Here's what I get for Python 2:

bytes-ascii
unicode-ascii
unicode-non-ascii:\u263a
\xe7-bytes

And for Python 3:

bytes-ascii
unicode-ascii
unicode-non-ascii:\u263a
\xe7-bytes

My original contribution on that part of the code was meant to show unicode ascii strings as ascii in the terminal instead of hiding them in a generic variable name, so this looks nice to me. 😉

I'm planning to repurpose test_unicode_idval_python to check for escaping. Should I change any of the other nearby tests to check for escaping?

Seems reasonable! 👍

Should I write a Hypothesis test for this code?

I'm 👍 on this, but would like to hear other's opinion too. Just in case, you should probably make that in a separate commit (in this same PR) in case anyone is 👎

Thanks again for all your work!

[The-Compiler]

I'm always 👍 for hypothesis! (Honestly, it's awesome. You think your stuff is well tested, spend 5 minutes throwing some hypothesis at it, and it finds bugs you never thought about)

[ceridwen]

I have a new test failure related to escaping on Python 3:

_____________________ test_escaped_parametrized_names_xml ______________________

testdir = <Testdir local('/tmp/pytest-of-cara/pytest-933/testdir/test_escaped_parametrized_names_xml0')>

    def test_escaped_parametrized_names_xml(testdir):
        testdir.makepyfile("""
            import pytest
            @pytest.mark.parametrize('char', ["\\x00"])
            def test_func(char):
                assert char
        """)
        result, dom = runandparse(testdir)
        assert result.ret == 0
        node = dom.find_first_by_tag("testcase")
>       node.assert_attr(name="test_func[#x00]")
E       assert {'name': 'test_func[\\x00]'} == {'name': 'test_func[#x00]'}

Should I change the assertion to match or do something else?

[nicoddemus]

You should change the test, the new behavior is to escape non-ascii chars. 😁

[ceridwen]

I don't know what to do with the tests that are failing to import Hypothesis or what's wrong with the format I used in CHANGELOG.rst. Someone is going to have to tell me what to do here.

[nicoddemus]

Hi @ceridwen,

1. For the failing tests in the xdist environments, there are separate environment definitions for them (here and here). I think adding hyphotesis as a dependency for testenv:py27-xdist should do the trick.
2. The linting environment is failing with Unknown target name: "@ceridwen"., which means that you created a target for ceridwen here but did not include the actual link among the others here;

I don't know how other core contributors track PR progress, but I rely on e-mail notifications which (unfortunately) are not triggered by new commits, that might be the reason why nobody commented on your changes until now. Sorry about that.

[ceridwen]

I changed the dependencies and the changelog. The fact that commits don't trigger email notifications is a real annoyance for me.

[nicoddemus]

I changed the dependencies and the changelog. The fact that commits don't trigger email notifications is a real annoyance for me.

Yep, BitBucket does, not sure why GitHub doesn't.

There are merge conflicts, could you please rebase your branch on features? Otherwise Travis and AppVeyor won't even start.

[nicoddemus]

Also .hypothesis should be added to .gitignore. 😁

[ceridwen]

Is there anything else?

[nicoddemus]

Looks great, thanks! 👍

I will merge this tomorrow unless someone still wants to comment on something. 😁

[nicoddemus]

Thanks again @ceridwen for the work and patience!

[RonnyPfannschmidt]

well done

[The-Compiler]

@ceridwen @nicoddemus FWIW I asked GitHub support about getting mails on PR updates, and they said they recorded it in their internal feature request tracker. I can't imagine I'm the first one to request that, but oh well... 😉

[ceridwen]

You're welcome, all.

I do wish that Github allowed updating pull requests, having the discussion for a pull request like this one in three separate threads (one issue, two pull requests) is annoying. I'm sorry for anyone who has to come read back through this discussion. Btw, someone should close the associated issue, #1351.

[coveralls]

Coverage increased (+0.02%) to 92.328% when pulling 1f46015 on ceridwen:features into da10451 on pytest-dev:features.