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

CreateAPITestCaseMixin using 2 saves on 2 different objects #29

Open
lukeaus opened this issue Jun 7, 2017 · 7 comments
Open

CreateAPITestCaseMixin using 2 saves on 2 different objects #29

lukeaus opened this issue Jun 7, 2017 · 7 comments

Comments

@lukeaus
Copy link

lukeaus commented Jun 7, 2017

Problem
I am getting an error that a django models.OneToOneField must be unique.

======================================================================
FAIL: test_create (work_ready.tests.TestWorkReady)
Send request to the create view endpoint, verify and return the response.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/site-packages/rest_assured/testcases.py", line 272, in test_create
    self.assertEqual(response.status_code, status.HTTP_201_CREATED, getattr(response, 'data', response))
AssertionError: {'worker': [u'This field must be unique.']}

The cause of this error is saving 2 different model instances with the same primary key.
What is happening is that in my model I have a django models.OneToOneField:

class WorkReady(models.Model):
    """Help Worker get ready for their first/next job"""
    worker = models.OneToOneField(Worker, related_name='work_ready')
    ...

Using CreateAPITestCaseMixin requires:

class (CreateAPITestCaseMixin, BaseRESTAPITestCase):
    ...

In BaseRESTAPITestCase.setUp() the object is getting created:

# create the object
self.object = self.get_object(self.get_factory_class())

So an object gets created in BaseRESTAPITestCase.setUp(), then another object gets created in CreateAPITestCaseMixin.test_create()

def test_create(self, data=None, **kwargs):
    response = self.get_create_response(data, **kwargs)   # second object gets created

You also can't not create an object in setUp() as CreateAPITestCaseMixin.test_create() requires self.object, which is set in BaseRESTAPITestCase.setUp().

def test_create(self, data=None, **kwargs):
    ...
    # self.object required here
    created = self.object.__class__.objects.get(
        **{self.lookup_field: self.get_lookup_from_response(response.data)})
    ...

Possible solution

  1. BaseRESTAPITestCase.setUp() should not create self.object when testing create method
  2. CreateAPITestCaseMixin.test_create() shouldn't rely on self.object to test that the object was created

Thanks for making django-rest-assured!

Happy to submit a PR for this issue.

@ydaniv
Copy link
Owner

ydaniv commented Jun 8, 2017

Hey @lukeaus, thanks for reporting!

I agree that the extra object creation seems redundant in this case, but what I can't figure is why your test is failing on primary key violation.

Why should the object created in setUp() and the object you create in test_create() have the same pk?

@lukeaus
Copy link
Author

lukeaus commented Jun 8, 2017

Just to be clear, it's not failing on the same primary key, its failing on the same foreign key on this model. The models.OneToOneField means that only one entry in WorkReady can have that foreignKey.

class WorkReady(models.Model):
"""Help Worker get ready for their first/next job"""
worker = models.OneToOneField(Worker, related_name='work_foo')

Here is my failing test implementation

class TestWorkReadyApi(ReadWriteRESTAPITestCaseMixin, BaseRESTAPITestCase, BaseTestClass):
    base_name = WorkReady.api_name()
    update_data = {'bar': '2'}

    def setUp(self):
        super(BaseTestClass, self).setUp()
        self.worker = WorkerFactory()
        self.user = self.worker.user
        self.login(self.user)
        self.object = self.get_object()

    def get_object(self):
        return WorkReadyFactory(worker=self.worker)

    def get_create_data(self):
        return {
            'worker': self.worker.pk,
            'bar': '1',
        }

Thanks

@ydaniv
Copy link
Owner

ydaniv commented Jun 12, 2017

So the problem here is your logic in get_create_data(). What you should be doing is:

def get_create_data(self):
    self.worker = WorkerFactory()
    return {
        'worker': self.worker.pk,
        'bar': 1
    }

And that should solve it (:

@lukeaus
Copy link
Author

lukeaus commented Jun 13, 2017

Thanks for the reply @ydaniv

Unfortunately that doesn't solve it :(

WorkerFactory() is not what is creating the object under test. WorkReadyFactory() is doing that.

My Worker instance created by WorkerFactory() needs to be created then passed to WorkReadyFactory().

Using the code you suggested gives me this error:

======================================================================
ERROR: test_update (work_ready.tests.TestWorkReady)
Send request to the update view endpoint, verify and return the response.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/app/project/workers/tests/tests_system_api.py", line 22, in setUp
    self.object = self.get_object()
  File "/app/project/workers/tests/tests_system_api.py", line 25, in get_object
    return WorkReadyFactory(worker=self.worker)
AttributeError: 'TestWorkReady' object has no attribute 'worker'

----------------------------------------------------------------------

Because my WorkReadyFactory requires WorkReady() to have already been run, I have to overwrite setUp()

self.object = self.get_object(self.get_factory_class())

In addition, it feels like a code smell to be setting self.xxx outside of init or setUp() methods.

Thanks

@ydaniv
Copy link
Owner

ydaniv commented Jun 14, 2017

@lukeaus ok, that was just a general idea.

So instead try setting self.worker2 = WorkerFactory() in setUp(), and in get_create_data() use this worker2.

Shouldn't this solve it?

@lukeaus
Copy link
Author

lukeaus commented Jun 14, 2017

@ydaniv

This works


    def get_create_data(self):
        self.worker2 = self.utils.data.create_worker(save=True)
        return {
            'worker': self.worker2.pk,
            'tfn': '1',
        }

However only works as a work around to the main issue. I only want one worker under test, not 2.

So the main issue still stands:

CreateAPITestCaseMixin.test_create() requires self.object to have been created via which uses
BaseRESTAPITestCase.get_object(), however CreateAPITestCaseMixin.create_data is used to create a second object. So creating 2 objects where only one is required. This has implications for models.OneToOneField and possibly other use cases as well (see initial post in this issue).

Possible solution:

  • Implement CreateAPITestCaseMixin.setUp() to call BaseRESTAPITestCase.setUp() with a no_create parameter so it doesn't implement this in BaseRESTAPITestCase.setUp() self.object = self.get_object(self.get_factory_class())

  • CreateAPITestCaseMixin to have a .model property that CreateAPITestCaseMixin.test_create() calls here instead of self.object ```
    created = self.object.class.objects.get(
    **{self.lookup_field: self.get_lookup_from_response(response.data)})

Could make backwards compatible by looking for CreateAPITestCaseMixin.model and then using that or self.object if no CreateAPITestCaseMixin.model.

Thanks

@ydaniv
Copy link
Owner

ydaniv commented Jun 19, 2017

@lukeaus still sounds to me like a minor optimization, but if you could put this together into a PR I'll gladly review and consider (:

Thanks!

lukeaus pushed a commit to lukeaus/django-rest-assured that referenced this issue Oct 3, 2017
lukeaus pushed a commit to lukeaus/django-rest-assured that referenced this issue Oct 3, 2017
lukeaus pushed a commit to lukeaus/django-rest-assured that referenced this issue Oct 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants