Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

ceridwen
Copy link
Contributor

I have two questions here:

  1. Is this what @RonnyPfannschmidt has in mind? My implementation is intended to ensure that the ids are always the str/bytes types on Python 2 and the str type on Python 3, and that non-ascii characters are always escaped as either bytes or Unicode, as appropriate.
  2. test_unicode_idval_python is currently testing that Unicode ids on Python 2 are replaced with generic ids. If I understand @RonnyPfannschmidt right, this behavior is changing, and Unicode ids will now be escaped instead. I'd either delete the test or repurpose it to test the new behavior. Arguably, some of the other nearby tests ought to be restructured or have cases added to them to check for correct escaping. What should I do here? I'd personally want to use Hypothesis to generate random data and make sure the code always returns something sensible, but adding a dependency like that for one test is probably too much.

@The-Compiler
Copy link
Member

I don't think it's much of a problem to add a testing dependency, and I think it'd really make sense for pytest to use hypothesis for some kinds of tests. It's just some kind of a circular dependency then? 😆 /cc @DRMacIver

@DRMacIver
Copy link

I don't think circular test dependencies are a problem. Hypothesis only depends on py.test for its tests as well.

@nicoddemus
Copy link
Member

For the record I don't see a problem either, we have several test-only dependencies. Is there any way to skip tests which use Hypothesis in case hypothesis is not installed? Just wondering.

elif isinstance(val, (float, int, str, bool, NoneType)):
return str(val)
elif isinstance(val, REGEX_TYPE):
return _escape_bytes(val.pattern) if isinstance(val.pattern, bytes) else val.pattern
return _escape_strings(val.pattern) if isinstance(val.pattern, bytes) else val.pattern
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given the new idea for unicode, i feel we no longer need the conditional, please check if thats the case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran the tests again with the conditional removed, and everything passed, so I assume it's okay.

@RonnyPfannschmidt
Copy link
Member

wrt poin2, since we now escape, i feel we should test the new code - however i realized we also need to target the features branch with that

@ceridwen
Copy link
Contributor Author

What tests do you want me to change? How do you feel about using Hypothesis? How do I move this patch to the features branch?

@davehunt
Copy link
Contributor

How do I move this patch to the features branch?

I'm not sure if GitHub allows you to edit the target of a pull request, but when you open a pull you can specify the target branch. For example features...ceridwen:master will show the changes between your master branch and the pytest features branch, and allows you to open a pull request.

@The-Compiler
Copy link
Member

You can do something like git rebase features; git push --force locally (ideally make a backup copy of the repo first), but then you'll need to close this PR and open a new one against features, as GitHub unfortunately doesn't support re-targeting PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants