-
Notifications
You must be signed in to change notification settings - Fork 116
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
Fix GetOrCreateSerializerMixin not working with pulp_domain #6078
Conversation
cf42aea
to
2b3742d
Compare
@@ -108,3 +115,27 @@ def test_ignored_fields_no_side_effects(): | |||
initial_data = {"field1": 1, "csrfmiddlewaretoken": 2} | |||
defined_fields = {"field1": 1} | |||
validate_unknown_fields(initial_data, defined_fields) | |||
|
|||
|
|||
class TestGetOrCreateSerializerMixin(TestCase): |
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.
Can you try if this creates valid pytest tests?
class TestGetOrCreateSerializerMixin(TestCase): | |
class TestGetOrCreateSerializerMixin(): |
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.
Looks like the setUp function may not be called with that name.
Also pytest really suggests to move to fixtures.
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.
Are there some docs or examples of unit tests with fixtures that hit the database? I was basing my tests off examples in pulp_container.
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 think this doc applies here.
https://pytest-django.readthedocs.io/en/latest/helpers.html#db
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.
Thanks, rewrote it the fixture way.
@@ -108,3 +115,27 @@ def test_ignored_fields_no_side_effects(): | |||
initial_data = {"field1": 1, "csrfmiddlewaretoken": 2} | |||
defined_fields = {"field1": 1} | |||
validate_unknown_fields(initial_data, defined_fields) | |||
|
|||
|
|||
class TestGetOrCreateSerializerMixin(TestCase): |
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.
Looks like the setUp function may not be called with that name.
Also pytest really suggests to move to fixtures.
2b3742d
to
c91e1e1
Compare
c91e1e1
to
56d8674
Compare
If we're backporting this, it would be nice to have an issue filed |
Can this PR be the issue? Don't feel that we will get much benefit out of creating a duplicate issue that has no new information. |
Backport to 3.39: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply ed27c95 on top of patchback/backports/3.39/ed27c950a713ed71493cd39d0b0baceabed33b36/pr-6078 Backporting merged PR #6078 into main
🤖 @patchback |
Backport to 3.28: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply ed27c95 on top of patchback/backports/3.28/ed27c950a713ed71493cd39d0b0baceabed33b36/pr-6078 Backporting merged PR #6078 into main
🤖 @patchback |
Backport to 3.49: 💚 backport PR created✅ Backport PR branch: Backported as #6106 🤖 @patchback |
Backport to 3.63: 💚 backport PR created✅ Backport PR branch: Backported as #6107 🤖 @patchback |
Backport to 3.68: 💚 backport PR created✅ Backport PR branch: Backported as #6108 🤖 @patchback |
pulp_domain is not processed in model's serializer since the value is auto-set as a context var on request time. This helper method would run into serializer errors if pulp_domain was specified as a natural key (which you need to do when supporting domains).