Refactor SQLs in xmlrpc (with tests) #159

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants
@atodorov
Contributor

atodorov commented Feb 26, 2017

More SQL refactoring into ORM, this time with working tests.

NOTE: the last commit where I add a test for xmlrpc.testcase.notification_remove_cc is a bit problematic. If I execute it on its own everything seems to work. If I execute make check pyunit produces a core dump. As of now I have not been able to identify why but I suspect it has to do with the creating/deleting the test objects themselves.

EDIT: found the culprit of the core dump - I wasn't deleting the cc emails and user objects created by my test. Already fixed.

tcms/xmlrpc/api/testplan.py
+ plan_id=_plan,
+ group_id=form.cleaned_data['env_group'].pk
+ )
+ )

This comment has been minimized.

@tkdchen

tkdchen Feb 28, 2017

Member

Lines from 692 to 699 can be simplified

new_maps = [TCMSEnvPlanMap(plan_id=plan_pk, group_id=form.cleaned_data['env_group'].pk) for plan_pk in plan_ids]
@tkdchen

tkdchen Feb 28, 2017

Member

Lines from 692 to 699 can be simplified

new_maps = [TCMSEnvPlanMap(plan_id=plan_pk, group_id=form.cleaned_data['env_group'].pk) for plan_pk in plan_ids]
tcms/xmlrpc/tests/test_testplan.py
class TestUpdate(test.TestCase):
- '''TODO: '''
+ ''' Tests the XMLRPM testplan.update method '''
+ @classmethod

This comment has been minimized.

@tkdchen

tkdchen Feb 28, 2017

Member

One blank line before @classmethod.

@tkdchen

tkdchen Feb 28, 2017

Member

One blank line before @classmethod.

tcms/xmlrpc/tests/test_testplan.py
+ self.assertEqual(self.plan_2.env_group.all()[0].pk, self.env_group_1.pk)
+
+ # and there are only 2 objects in the many-to-many table
+ self.assertEqual(TCMSEnvPlanMap.objects.count(), 2)

This comment has been minimized.

@tkdchen

tkdchen Feb 28, 2017

Member

It looks lines from 423 to 430 test the setUpClass. Is this really necessary? Why?

@tkdchen

tkdchen Feb 28, 2017

Member

It looks lines from 423 to 430 test the setUpClass. Is this really necessary? Why?

This comment has been minimized.

@atodorov

atodorov Feb 28, 2017

Contributor

This is checking whether or not the initial condition is as we expect it to be but thinking about it it is probably not necessary. These lines will break only after modification of the setup methods and I guess the person doing that should be able to figure out that their change broke something and update the tests. I will remove these lines.

@atodorov

atodorov Feb 28, 2017

Contributor

This is checking whether or not the initial condition is as we expect it to be but thinking about it it is probably not necessary. These lines will break only after modification of the setup methods and I guess the person doing that should be able to figure out that their change broke something and update the tests. I will remove these lines.

tcms/xmlrpc/tests/test_testplan.py
+
+ # and there are still only 2 objects in the many-to-many table
+ # iow no dangling objects left
+ self.assertEqual(TCMSEnvPlanMap.objects.count(), 2)

This comment has been minimized.

@tkdchen

tkdchen Feb 28, 2017

Member

A stable way is

self.assertEqual(TCMSEnvPlanMap.objects.filter(plan__in=[self.plan_1, self.plan_2]).count(), 2)

This can avoid special cases if there are maps added by other tests but not deleted.

@tkdchen

tkdchen Feb 28, 2017

Member

A stable way is

self.assertEqual(TCMSEnvPlanMap.objects.filter(plan__in=[self.plan_1, self.plan_2]).count(), 2)

This can avoid special cases if there are maps added by other tests but not deleted.

tcms/xmlrpc/tests/test_testcase.py
+
+class TestNotificationRemoveCC(test.TestCase):
+ ''' Tests the XMLRPM testcase.notication_remove_cc method '''
+ @classmethod

This comment has been minimized.

@tkdchen

tkdchen Feb 28, 2017

Member

One blank line before @classmethod.

@tkdchen

tkdchen Feb 28, 2017

Member

One blank line before @classmethod.

tcms/xmlrpc/tests/test_testcase.py
+
+
+class TestNotificationRemoveCC(test.TestCase):
+ ''' Tests the XMLRPM testcase.notication_remove_cc method '''

This comment has been minimized.

@tkdchen

tkdchen Feb 28, 2017

Member

Please use double quote to quote docstring, e.g.

class TestNotificationRemoveCC(test.TestCase):
    """Tests the XMLRPM testcase.notication_remove_cc method"""
@tkdchen

tkdchen Feb 28, 2017

Member

Please use double quote to quote docstring, e.g.

class TestNotificationRemoveCC(test.TestCase):
    """Tests the XMLRPM testcase.notication_remove_cc method"""

This comment has been minimized.

@atodorov

atodorov Feb 28, 2017

Contributor

You have used single quotes for doc strings far more often than double quotes. I merely looked at what was already there. So which one style do you prefer so I can open an issue to update the other one ?

@atodorov

atodorov Feb 28, 2017

Contributor

You have used single quotes for doc strings far more often than double quotes. I merely looked at what was already there. So which one style do you prefer so I can open an issue to update the other one ?

This comment has been minimized.

@tkdchen

tkdchen Mar 1, 2017

Member

Oh, many code including those docstrings quoted by ''' are really old and written several years ago. For now, double quote should be used to quote docstrings. This is the style I've been using in all my Python code. Sorry for confusion.

It would be fine to open an issue to fixed them all at once. Feel free to do that and fix them if you like.

@tkdchen

tkdchen Mar 1, 2017

Member

Oh, many code including those docstrings quoted by ''' are really old and written several years ago. For now, double quote should be used to quote docstrings. This is the style I've been using in all my Python code. Sorry for confusion.

It would be fine to open an issue to fixed them all at once. Feel free to do that and fix them if you like.

This comment has been minimized.

@atodorov

atodorov Mar 1, 2017

Contributor

Fixed and rebased to latest

@atodorov

atodorov Mar 1, 2017

Contributor

Fixed and rebased to latest

+ def tearDownClass(cls):
+ cls.testcase.emailing.cc_list.all().delete()
+ cls.testcase.delete()
+ cls.user.delete()

This comment has been minimized.

@tkdchen

tkdchen Feb 28, 2017

Member

Also better call super's tearDownClass.

@tkdchen

tkdchen Feb 28, 2017

Member

Also better call super's tearDownClass.

This comment has been minimized.

@atodorov

atodorov Feb 28, 2017

Contributor

This has been updated, however you don't call super().tearDownClass() in almost all of the other tests. I will open an issue to fix that.

@atodorov

atodorov Feb 28, 2017

Contributor

This has been updated, however you don't call super().tearDownClass() in almost all of the other tests. I will open an issue to fix that.

This comment has been minimized.

@tkdchen

tkdchen Mar 1, 2017

Member

I see the issue you opened. It is really necessary to fix those tests code.

@tkdchen

tkdchen Mar 1, 2017

Member

I see the issue you opened. It is really necessary to fix those tests code.

tcms/xmlrpc/tests/test_testplan.py
class TestUpdate(test.TestCase):
- '''TODO: '''
+ ''' Tests the XMLRPM testplan.update method '''

This comment has been minimized.

@tkdchen

tkdchen Feb 28, 2017

Member

Same as above

@tkdchen

tkdchen Feb 28, 2017

Member

Same as above

+ cls.product.delete()
+ cls.product.classification.delete()
+ cls.env_group_1.delete()
+ cls.env_group_2.delete()

This comment has been minimized.

@tkdchen

tkdchen Feb 28, 2017

Member

Same as above, call super's tearDownClass.

@tkdchen

tkdchen Feb 28, 2017

Member

Same as above, call super's tearDownClass.

This comment has been minimized.

@atodorov

atodorov Feb 28, 2017

Contributor

fixed

@atodorov

atodorov Feb 28, 2017

Contributor

fixed

tcms/xmlrpc/api/testcase.py
- cursor.execute(sql, tc_ids)
- transaction.commit_unless_managed()
+
+ for tc in TestCase.objects.filter(pk__in=tc_ids).iterator():

This comment has been minimized.

@tkdchen

tkdchen Feb 28, 2017

Member

Here, the queried data size can be reduced by

for tc in TestCase.objects.filter(pk__in=tc_ids).only('pk').iterator():

Removing CC here only requires the relationship between TestCase and TestCaseEmailSettings, no other TestCase attributes are needed.

@tkdchen

tkdchen Feb 28, 2017

Member

Here, the queried data size can be reduced by

for tc in TestCase.objects.filter(pk__in=tc_ids).only('pk').iterator():

Removing CC here only requires the relationship between TestCase and TestCaseEmailSettings, no other TestCase attributes are needed.

tcms/xmlrpc/tests/test_testcase.py
+ def test_remove_existing_cc(self):
+ # first verify that testcase has the default CC listed
+ self.assertEqual(self.testcase.emailing.cc_list.count(), 1)
+ self.assertEqual(self.testcase.emailing.cc_list.all()[0].email, self.default_cc)

This comment has been minimized.

@tkdchen

tkdchen Feb 28, 2017

Member

Same thought as another comment in another commit. Why to verify the data validation of setUpClass?

@tkdchen

tkdchen Feb 28, 2017

Member

Same thought as another comment in another commit. Why to verify the data validation of setUpClass?

tcms/xmlrpc/tests/test_testcase.py
+ XmlrpcTestCase.notification_remove_cc(self.http_req, self.testcase.pk, [self.default_cc])
+
+ # now verify that the CC email has been removed
+ self.assertEqual(self.testcase.emailing.cc_list.count(), 0)

This comment has been minimized.

@tkdchen

tkdchen Feb 28, 2017

Member

Also, query the count of cc of self.testcase explicitly.

@tkdchen

tkdchen Feb 28, 2017

Member

Also, query the count of cc of self.testcase explicitly.

This comment has been minimized.

@atodorov

atodorov Feb 28, 2017

Contributor

I don't understand this comment, can you clarify what do you mean.

@atodorov

atodorov Feb 28, 2017

Contributor

I don't understand this comment, can you clarify what do you mean.

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Feb 28, 2017

Member

Please mark those commits be related to #148 in commit message, that fix commit_unless_managed.

In addition, please change the assertions by swapping expected and actual value. In Nitrate, expected value should be passed as the first parameter to assertion methods.

self.assertEqual(expected, actual)
Member

tkdchen commented Feb 28, 2017

Please mark those commits be related to #148 in commit message, that fix commit_unless_managed.

In addition, please change the assertions by swapping expected and actual value. In Nitrate, expected value should be passed as the first parameter to assertion methods.

self.assertEqual(expected, actual)
@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Mar 1, 2017

Contributor

@tkdchen rebased to latest and ready to go. For some reason Travis CI failed but doesn't show me any logs.

Contributor

atodorov commented Mar 1, 2017

@tkdchen rebased to latest and ready to go. For some reason Travis CI failed but doesn't show me any logs.

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Mar 2, 2017

Member

Please fix the commit message according to comment added to #166

Member

tkdchen commented Mar 2, 2017

Please fix the commit message according to comment added to #166

atodorov added some commits Feb 25, 2017

Add test for xmlrpc.testplans.update (env_group update)
Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
Refactor TP_CLEAR_ENV_GROUP and TP_ADD_ENV_GROUP SQLs into ORM.
also the test added in the previous commit was generating an error
which is now fixed (related to #148). The error was:

'module' object has no attribute 'commit_unless_managed'

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
Refactor TC_REMOVE_CC SQL into ORM
by updating xmlrpc.testcase.notification_remove_cc() method. I've
made this method very similar to notification_add_cc() and
notification_get_cc() methods. Also added a test!

Related to #146

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>

@atodorov atodorov referenced this pull request Mar 2, 2017

Closed

Fix sqls #167

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Mar 2, 2017

Member

Segmentation fault (core dumped) happens in Travis. I have run make check, all tests pass.

Member

tkdchen commented Mar 2, 2017

Segmentation fault (core dumped) happens in Travis. I have run make check, all tests pass.

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Mar 2, 2017

Member

Merged. Thanks.

Member

tkdchen commented Mar 2, 2017

Merged. Thanks.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Mar 2, 2017

Contributor

closing b/c merged manually

Contributor

atodorov commented Mar 2, 2017

closing b/c merged manually

@atodorov atodorov closed this Mar 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment