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.

Sign up for GitHub

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

Make 'nailgun.entity_fields.URLField.gen_value' method always generates unique value #190

Merged
merged 1 commit into from
Aug 31, 2015
Merged

Make 'nailgun.entity_fields.URLField.gen_value' method always generates unique value #190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 4 additions & 16 deletions nailgun/entities.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

"""
from datetime import datetime
from fauxfactory import gen_alpha, gen_alphanumeric, gen_url
from fauxfactory import gen_alphanumeric
from nailgun import client, entity_fields
from nailgun.entity_mixins import (
Entity,
Expand Down Expand Up @@ -1496,9 +1496,9 @@ def __init__(self, server_config=None, **kwargs):
def create_missing(self):
"""Customize the process of auto-generating instance attributes.

By default, :meth:`nailgun.entity_fields.URLField.gen_value` does not
return especially unique values. This is problematic, as all domain
names must be unique.
By default, :meth:`nailgun.entity_fields.StringField.gen_value` can
produce strings in both lower and upper cases, but domain name should
be always in lower case due logical reason.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this. I must've copied and pasted. Blech!


"""
if not hasattr(self, 'name'):
Expand Down Expand Up @@ -2245,18 +2245,6 @@ def __init__(self, server_config=None, **kwargs):
self._meta = {'api_path': 'api/v2/media', 'server_modes': ('sat')}
super(Media, self).__init__(server_config, **kwargs)

def create_missing(self):
"""Give the ``path_`` instance attribute a value if it is unset.

By default, :meth:`nailgun.entity_fields.URLField.gen_value` does not
return especially unique values. This is problematic, as all media must
have a unique path.

"""
if not hasattr(self, 'path_'):
self.path_ = gen_url(subdomain=gen_alpha())
return super(Media, self).create_missing()

def create_payload(self):
"""Wrap submitted data within an extra dict and rename ``path_``.

Expand Down
3 changes: 2 additions & 1 deletion nailgun/entity_fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

"""
from fauxfactory import (
gen_alpha,
gen_boolean,
gen_choice,
gen_date,
Expand Down Expand Up @@ -265,4 +266,4 @@ class URLField(StringField):

def gen_value(self):
"""Return a value suitable for a :class:`URLField`."""
return gen_url()
return gen_url(subdomain=gen_alpha())
Copy link
Member

Choose a reason for hiding this comment

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

9gag and 4chan fans might get upset about this solution..how about this?

return gen_url(subdomain=gen_alphanumeric())

also, how about including a . to support multiple-domain format (like foo.bar.org)?
of course it must not be at the beginning or at the end of the string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not restrict or introduce any limitation to possible URL, so if you like to use particular URL in your specific test case - just do it, but for generic scenario (for create_missing() method) I prefer to use most basic generator method, so it will be applicable for most possible situations

Copy link
Contributor

Choose a reason for hiding this comment

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

@rplevka You're right. It'd be good to generate more "interesting" data. I didn't both pushing for the change, though, because this PR really just works around issues in the underlying fauxfactory library. An even better solution is to make the gen_url function produce a greater variety of URLs and obey RFC 2606.

Copy link
Member

Choose a reason for hiding this comment

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

[...] this PR really just works around issues in the underlying fauxfactory library.

What are they? Is there an issue filed to track them?

Copy link
Contributor

Choose a reason for hiding this comment

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

@omaciel There's two issues:

  • gen_url, gen_email and possibly others generate real values that might be in use in the wild. For example, gen_url might spit out test.com.
  • gen_url doesn't produce especially random values. That's why we're calling gen_url(subdomain=gen_alpha()) instead of gen_url() here in NailGun.

omaciel/fauxfactory#26 is describes how gen_email produces "real" values. There aren't issues for the other methods that produce "real" values. There's not an issue for gen_url's lack of randomness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@omaciel from my observation gen_url method give us not so many variations of inputs and such data become similar pretty fast. Let's say, it generate:
test.com, example.com, test.biz, example.biz and few more, so even for 10 tests there is chance that we will have duplicated entry. Using proposed approach we can decrease such chances in hundreds times

17 changes: 0 additions & 17 deletions tests/test_entities.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,23 +715,6 @@ def test_lifecycle_environment_v3(self):
with self.assertRaises(entities.APIResponseError):
entity.create_missing()

def test_media_v1(self):
"""Test ``Media()``."""
entity = entities.Media(self.cfg)
with mock.patch.object(EntityCreateMixin, 'create_raw'):
with mock.patch.object(EntityReadMixin, 'read_raw'):
entity.create_missing()
self.assertTrue('path_' in entity.get_values())

def test_media_v2(self):
"""Test ``Media(path_=…)``."""
path = gen_string('alphanumeric')
entity = entities.Media(self.cfg, path_=path)
with mock.patch.object(EntityCreateMixin, 'create_raw'):
with mock.patch.object(EntityReadMixin, 'read_raw'):
entity.create_missing()
self.assertEqual(entity.path_, path)

def test_repository_v1(self):
"""Test ``Repository(content_type='docker')``."""
entity = entities.Repository(self.cfg, content_type='docker')
Expand Down