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

Support django's URLField #1388

Closed
sobolevn opened this issue Jul 8, 2018 · 8 comments
Closed

Support django's URLField #1388

sobolevn opened this issue Jul 8, 2018 · 8 comments
Labels
enhancement it's not broken, but we want it to be better

Comments

@sobolevn
Copy link
Member

sobolevn commented Jul 8, 2018

While using hypothesis quite successfully for my django apps, I have faced a minor issue with URLField.

Currently it is impossible to generate random URLs with hypothesis. And URLField is not supported.

Here's my example:

# my_app/models.py

class Repository(models.Model):
     url = models.URLField()

# tests/test_repository.py

@given(repo_prop=models(Repository))
def test_user_model(repo_prop):
    """Tests that every possible Repository can be created."""
    assert repo_prop.id > 0

Which gives me this error:

==================================== ERRORS ====================================
___ ERROR collecting tests/test_vcs_app/test_models/test_repository_model.py ___
tests/test_vcs_app/test_models/test_repository_model.py:17: in <module>
    @given(repo_prop=models(Repository))
.venv/lib/python3.6/site-packages/hypothesis/extra/django/models.py:215: in models
    % (u's' if missed else u'', u', '.join(missed), model.__name__))
E   hypothesis.errors.InvalidArgument: Missing arguments for mandatory fields url for model Repository
___ ERROR collecting tests/test_vcs_app/test_models/test_repository_model.py ___
tests/test_vcs_app/test_models/test_repository_model.py:17: in <module>
    @given(repo_prop=models(Repository))
.venv/lib/python3.6/site-packages/hypothesis/extra/django/models.py:215: in models
    % (u's' if missed else u'', u', '.join(missed), model.__name__))
E   hypothesis.errors.InvalidArgument: Missing arguments for mandatory fields url for model Repository

Current solution

I am using a workaround with faker:

from django.db.models import URLField
from hypothesis.extra.django.models import add_default_field_mapping
from hypothesis.extra.fakefactory import fake_factory

add_default_field_mapping(URLField, fake_factory('uri'))

After this fix it works, but it feels like some dirty hack.

Desired solution

I guess, it would be great to have st.urls() to generate URLs with corner cases.
And also map this strategy right to the django's URLField by default.

I know that @kxepal has some working prototype that does exactly that.

@kxepal
Copy link
Member

kxepal commented Jul 8, 2018

Indeed I made urls strategy once upon a time: https://gist.github.com/kxepal/bceffb0f909a2339449f1e258d0fd433 (sorry for unittest, this code is about three years old). Do we want to have such strategy out of the box? I could try to bring that code back to live and contribute it.
This will require some more work to support IDNA hostnames and integrate it with email strategy which relays upon hostname definition.

@DRMacIver
Copy link
Member

Indeed I made urls strategy once upon a time: https://gist.github.com/kxepal/bceffb0f909a2339449f1e258d0fd433 (sorry for unittest, this code is about three years old). Do we want to have such strategy out of the box? I could try to bring that code back to live and contribute it.
This will require some more work to support IDNA hostnames and integrate it with email strategy which relays upon hostname definition.

I'd be keen.

Also, feel free to submit an initial low-pass version which you can refine later. As long as we get the API right up front, the implementation only needs to be good enough to be useful!

@Zac-HD
Copy link
Member

Zac-HD commented Dec 21, 2018

We can also put a prototype urls() strategy in hypothesis.provisional, which is pretty much all strategies that are useful for Django but not ready to be public API directly. Building one out of the provisional.domains() strategy shouldn't be too hard!

@grigoriosgiann
Copy link
Contributor

Hey there!
I'd love to work on this.

@Zac-HD
Copy link
Member

Zac-HD commented Dec 26, 2018

Go for it! Feel free to ping me if you get stuck 😄

Per our usual policy, the issue is yours for a week before it's open to anyone again.

@grigoriosgiann
Copy link
Contributor

Hey @Zac-HD!
I've made some progress: master...GrigoriosGiann:add-url-strategy
For the record, I focused only on HTTP/HTTPS URLs.

What I have added:

  • path generation
  • port generation

What I haven't done:

  • utf-8 support (only ASCII URLs for now)
  • query support (the part after the ?)
  • using an IP address in lieu of a domain
  • using mixed (upper and lowercase) case hexadecimal for escaping
  • percent encoding random safe characters

For the hosts, I'm reusing the domains() strategy.
For the path, non safe characters get percent-encoded according to RFC 1738.
Right now I am encoding ;, :, @, &, =, even though they are technically valid for HTTP URLs.

Things I am not sure about

  • I am unsure if it's ok not encoding some characters that RFC 1738 allows, or I should do a typical URI percent encoding. The former leads to some weirder paths that include commas, and parenthesis, which are less common, but valid.
  • Before URL encoding, I only choose printable characters for the path.

@Zac-HD
Copy link
Member

Zac-HD commented Jan 3, 2019

Nice! I'd suggest opening a PR with this and the Django connection too 😄

I'll have some comments on the detail later, but overall this is a perfect example of a provisional strategy - not perfect, but much better than the status quo (and we can always improve it later). IMO it's better than the domains strategy!

@Zac-HD
Copy link
Member

Zac-HD commented Jan 7, 2019

Closed by #1736 🎉

@Zac-HD Zac-HD closed this as completed Jan 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement it's not broken, but we want it to be better
Projects
None yet
Development

No branches or pull requests

5 participants