Properly escape test names when setting PYTEST_CURRENT_TEST environment variable

[nicoddemus]

Fix #2644

[coveralls]

Coverage decreased (-0.008%) to 91.807% when pulling 1deac2e on nicoddemus:issue-2644 into 02da156 on pytest-dev:master.

[The-Compiler]

I wonder what other stuff null bytes in node IDs could break though...

[RonnyPfannschmidt]

this needs a followup as test ids should be ascii to begin with, if they are non-ascii then we messed up somewhere else

please lets add a comment that this is only a workaround so the real bug doesnt affect people directly

[The-Compiler]

I think _ascii_escaped does nothing here (like Ronny says, _ascii_escaped is probably called on all node IDs already) - however, null bytes are in ASCII 😉

I think the proper solution for this is for _ascii_escaped to escape null bytes (probably as \x00 for consistency though).

[nicoddemus]

this needs a followup as test ids should be ascii to begin with, if they are non-ascii then we messed up somewhere else
I think _ascii_escaped does nothing here

Argh you guys are correct, it was a brain fart on my part: nodeids are already turned into ascii during parametrization. The problem here is that "null bytes" are ascii, so they are left unchanged. This works fine everywhere else it seems, but Python 2 (and not Python 3) doesn't like null bytes in environment variables, due to implementation details (I assume).

I would not like to touch _ascii_escaped to remove null bytes at the moment, given that it seems to work fine everywhere else so far; we might decide that it is safer for the long term to do so, but I would rather do it in features if we decide to do so to ensure we don't break anything now.

I changed it to only remove the null bytes from the string that will be set as environment variable, and only in Python 2. Let me know what you guys think.

[RonnyPfannschmidt]

a valid change, but it will also mean that the ids we store will not match the real test name, in addition i suspect we cant really support ids with null bytes as they cant be properly transferred anywhere else or even entered from the cli

[The-Compiler]

So does this _ascii_escaped call do anything here? I think it does nothing as we discovered, and if that's true, it should probably be removed to not cause further confusion.

[nicoddemus]

Sorry, I didn't push the changes yet (I wrote my comment meaning to push right away, but got dragged by other things).

[nicoddemus]

Just pushed it

[nicoddemus]

but it will also mean that the ids we store will not match the real test name

What do you mean by "will not"? I mean: we already "ascii escape" node ids for some time now, so I don't see why my PR changed this in any way.

in addition i suspect we cant really support ids with null bytes as they cant be properly transferred anywhere else or even entered from the cli

I see what you mean, but as I said I'm not comfortable doing this in this PR (or in master for that matter) because we might break things that are working. I'm OK with changing that in features (and in a separate PR) if we think that's a good idea.

[The-Compiler]

I agree with @nicoddemus, and I think it'd be a good idea to escape null bytes in node IDs in general (in features). I'm kind of surprised this didn't bite us earlier, considering that node IDs are used in various places.

Actually, this makes me wonder whether we should just escape all ASCII values before 0x20 (space)... Those are all "special" chars, and some of them invisible (but it also contains things like tab).

[coveralls]

Coverage decreased (-0.008%) to 91.807% when pulling 7703dc9 on nicoddemus:issue-2644 into 02da156 on pytest-dev:master.

[nicoddemus]

Moved this discussion over to #2649