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

Jump to bottom

Implement factories for DCIM DeviceRole, DeviceType, Manufacturer, and Platform #2594

Merged
merged 21 commits into from
Oct 27, 2022
Merged

Implement factories for DCIM DeviceRole, DeviceType, Manufacturer, and Platform #2594

merged 21 commits into from
Oct 27, 2022

Conversation

jathanism
Copy link
Contributor

@jathanism jathanism commented Oct 7, 2022 •
edited
Loading

Closes: #1334, Related: #2282

What's Changed

This implements DeviceRoleFactory, DeviceTypeFactory, ManufacturerFactory, and PlatformFactory.

  • Implemented hard-coding a list of desired manufacturer names, derived platform and device type names from that e.g. Manufacturer.name = "Cisco" would result in Platform.name = "Cisco Platform 1" and DeviceType.name = "Cisco DeviceType 1".
  • Refactored a large share of tests that create their own bespoke objects of these types, however, have not yet completed it. This is a big step forward, however, especially in reducing duplication of code in the test suite.

TODO

  • Explanation of Change(s)
  • Added change log fragment(s) (for more information see the documentation)
  • Attached Screenshots, Payload Example
  • Unit, Integration Tests
  • Documentation Updates (when adding/changing features)
  • Example Plugin Updates (when adding/changing features)
  • Outline Remaining Work, Constraints from Design

@jathanism jathanism changed the base branch from develop to next October 7, 2022 21:56
@jathanism jathanism changed the title mplement factories for DCIM DeviceType, Manufacturer, and Platform Implement factories for DCIM DeviceType, Manufacturer, and Platform Oct 7, 2022
gsnider2195
gsnider2195 reviewed Oct 11, 2022
View reviewed changes
changes/2594.added Outdated Show resolved Hide resolved
@gsnider2195
Copy link
Contributor

Can you add these to nautobot/core/management/commands/generate_test_data.py

@jathanism jathanism marked this pull request as draft October 11, 2022 22:55
@jathanism jathanism force-pushed the jathanism-device_type-factory branch from 397a840 to 041bde7 Compare October 12, 2022 23:11
glennmatthews
glennmatthews reviewed Oct 14, 2022
View reviewed changes
changes/2594.added Outdated Show resolved Hide resolved
nautobot/dcim/factory.py Show resolved Hide resolved
nautobot/dcim/tests/test_filters.py Show resolved Hide resolved
nautobot/dcim/tests/test_filters.py Outdated Show resolved Hide resolved
@jathanism jathanism marked this pull request as ready for review October 17, 2022 22:30
@jathanism jathanism changed the title Implement factories for DCIM DeviceType, Manufacturer, and Platform Implement factories for DCIM DeviceRole, DeviceType, Manufacturer, and Platform Oct 17, 2022
@jathanism
Implement factories for DCIM DeviceType, Manufacturer, and `Platf…
856e4e2 
…orm`
@jathanism
Changelog file
eec2ae1
@jathanism
Refined factories to use LazyFunction vs. Params + LazyAttribute
98abf19 
- Added object creation to `generate_test_data
@jathanism
Refactor the factories to have more human-readable names
a4288e3 
- `Manufacturer` will be named from a predefined list of known network vendors
- `Platform` will only set `napalm_driver` if a `manufacturer` is set
- `Platform` will be named based on the `manufacturer.name`
- `Platform.napalm_driver` will be selected from a mapping of known driver names
- `DeviceType` will be named based on the `manufacturer.name`
@jathanism
Refactored DCIM test_forms.py to use factories.
f73675f
@jathanism
Implemented DeviceRoleFactory and added it to generate_test_data
9a6973e 
- Also tweaked the generation a bit for the other factories I've added.
@jathanism
Refactor test_api to use my new factories.
14b8ef0
@jathanism
Refactored test_filters to use new factories.
d32dc47 
- Updated new factories to have `description` be only a single sentence to simplify filter testing.
@jathanism
Fix GraphQL tests to work with new changes.
b239b2d
@jathanism
Fix test_forms to be more deterministic given random test data.
2564c05
@jathanism
Make pylint happy.
bd27026
@jathanism
Got filter and views tests working...
35f1a39
@jathanism
Fix VM and ConfigContext filter tests.
5de9a71 
Still broken:

```
======================================================================
FAIL: test_api_docs (nautobot.utilities.tests.test_api.APIDocsTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jathan/sandbox/src/nautobot/nautobot/utilities/tests/test_api.py", line 146, in test_api_docs
    self.assertEqual(response.status_code, 200)
AssertionError: 500 != 200
```
@jathanism
PR feedback.
dcc3663
@jathanism jathanism force-pushed the jathanism-device_type-factory branch from 1357d8c to dcc3663 Compare October 19, 2022 22:17
@glennmatthews glennmatthews mentioned this pull request Oct 21, 2022
Merged
2 tasks
@jathanism @gsnider2195
Update nautobot/dcim/tests/test_filters.py
8fd33b5 
Co-authored-by: Gary Snider <75227981+gsnider2195@users.noreply.github.com>
@gsnider2195
Copy link
Contributor

This wasn't a result of any changes you made but I noticed this test failing while I was running tests for your PR. Should we fix it in your branch since it's also related to factory changes?

Seed: nautobot-server generate_test_data --flush --seed 'UzDYiR8TqHcoqiWE'
Failing test:

======================================================================
FAIL: test_search (nautobot.extras.tests.test_filters.StatusTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/source/nautobot/extras/tests/test_filters.py", line 1407, in test_search
    self.assertEqual(self.filterset(params, self.queryset).qs.count(), 1)
AssertionError: 2 != 1

>>> from nautobot.extras.filters import StatusFilterSet
>>> StatusFilterSet({'q': 'active'}, Status.objects.all()).qs
<StatusQuerySet [<Status: Active>, <Status: ComplexActive>]>

@gsnider2195
Copy link
Contributor

gsnider2195 commented Oct 27, 2022 •
edited
Loading

This wasn't a result of any changes you made but I noticed this test failing while I was running tests for your PR. Should we fix it in your branch since it's also related to factory changes?

As discussed I'll be fixing this

glennmatthews
glennmatthews approved these changes Oct 27, 2022
View reviewed changes
nautobot/dcim/tests/test_views.py
# FIXME(jathan): This has to be replaced with# `get_deletable_object` and
# `get_deletable_object_pks` but this is a workaround just so all of these objects are
# deletable for now.
DeviceType.objects.all().delete()
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this be a problem as soon as we add Device factories, since Device.device_type uses models.PROTECT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! Thus the comments! This is really just to get this merged in so we don't keep kicking the merge conflicts can down the road.

nautobot/dcim/tests/test_views.py
# factories.
@override_settings(EXEMPT_VIEW_PERMISSIONS=["*"])
def test_bulk_edit_objects_with_constrained_permission(self):
DeviceType.objects.exclude(model__startswith="Test Device Type").delete()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same concern here and below. I totally understand this is gnarly code to work with but just want to make sure we're not setting ourselves up for more pain in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Agreed. Definitely have to replace all of this as we get closer to refactoring everything to use and expect factory-generated fixtures.

nautobot/extras/tests/test_filters.py
params = {"role": [device_roles[0].slug, device_roles[1].slug]}
self.assertEqual(self.filterset(params, self.queryset).qs.count(), 2)
self.assertEqual(self.filterset(params, self.queryset).qs.count(), len(device_roles))
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems weird to me that these changes are needed - as written it sure looks like we should always have exactly two results here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there were cases where it was sometimes resulting in a result of 1 so I made it where the roles being selected weren't adjacent and then relied on the count there instead of hard-coding the integer value expected. Meh.

@jathanism jathanism merged commit eedc6c8 into next Oct 27, 2022
@jathanism jathanism deleted the jathanism-device_type-factory branch October 27, 2022 18:40
@bryanculver bryanculver mentioned this pull request Nov 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glennmatthews glennmatthews glennmatthews approved these changes

@gsnider2195 gsnider2195 gsnider2195 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
Nautobot Core ⚙️
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider using a factory library to generate model instances for testing purposes
3 participants
@jathanism @gsnider2195 @glennmatthews