-
Notifications
You must be signed in to change notification settings - Fork 263
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
Replace tag/status fixtures with factories #2593
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -837,12 +834,14 @@ def setUpTestData(cls): | |||
) | |||
|
|||
def test_name(self): | |||
params = {"name": VLAN.objects.all().values_list("name", flat=True)[:2]} | |||
self.assertEqual(self.filterset(params, self.queryset).qs.count(), 2) | |||
names = list(VLAN.objects.all().values_list("name", flat=True))[:2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm realizing in cases like this it's gonna be a tad more optimal to slice the queryset first, and cast to a list second, assuming we have more than a handful of VLAN objects, that is.
For example:
names = list(VLAN.objects.all().values_list("name", flat=True))[:2] | |
names = list(VLAN.objects.all().values_list("name", flat=True)[:2]) |
Perhaps since we're starting to repeat this pattern a lot as we refactor to use the fixture factories, we might consider factoring it into a test case helper method? Just thinking aloud. Please don't change it right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid plan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -158,7 +158,7 @@ class Meta: | |||
) | |||
|
|||
# note that name is *not* globally unique | |||
name = factory.Faker("color_name") | |||
name = factory.LazyFunction(lambda: f"{faker.Faker().color_name()}{faker.Faker().pyint()}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this make sense to be a LazyAttributeSequence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could do, then the names would be (random color + sequential number) instead of (random color + random number). I don't feel strongly about one versus the other, I just needed to add something because color names alone weren't proving to be unambiguous enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought was this line would be easier to read as:
name = factory.LazyAttributeSequence(lambda n: f"{faker.Faker().color_name()}{n}")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easier to read but guarantees that names are globally unique, which isn't actually a requirement of the model.
Co-authored-by: Gary Snider <75227981+gsnider2195@users.noreply.github.com>
Closes: #DNE
What's Changed
clear_status_choices
that were made in Add RouteTarget and VRF factories. Add TenancyFilterTestCaseMixin and refactor #2514 and instead just remove the tests for this function.TODO