Refactor SQL queries in ajax.py to Django queries #232

asankov · 2018-02-23T23:53:58Z

As discussed in #222 the SQL queries in ajax.py needed to be refactored into Django queries.

coveralls · 2018-02-24T00:00:24Z

Pull Request Test Coverage Report for Build 749

7 of 29 (24.14%) changed or added relevant lines in 1 file are covered.
5 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.1%) to 63.762%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tcms/core/ajax.py	7	29	24.14%

Files with Coverage Reduction	New Missed Lines	%
tcms/core/ajax.py	1	10.17%
tcms/core/models/fields.py	2	100.0%
tcms_api/containers.py	2	69.79%

Totals
Change from base Build 747:	-0.1%
Covered Lines:	7471
Relevant Lines:	10992

💛 - Coveralls

codecov-io · 2018-02-24T00:30:41Z

Codecov Report

Merging #232 into master will increase coverage by 0.14%.
The diff coverage is 27.77%.

@@            Coverage Diff             @@
##           master     #232      +/-   ##
==========================================
+ Coverage   63.81%   63.96%   +0.14%     
==========================================
  Files         139      143       +4     
  Lines       10970    11284     +314     
  Branches     1664     1713      +49     
==========================================
+ Hits         7001     7218     +217     
- Misses       3494     3570      +76     
- Partials      475      496      +21

Impacted Files	Coverage Δ
tcms/core/ajax.py	`13.21% <27.77%> (-0.13%)`	⬇️
tcms/core/models/fields.py	`74.07% <0%> (-25.93%)`	⬇️
tcms/core/views/index.py	`66.66% <0%> (-22.23%)`	⬇️
tcms/profiles/views.py	`57.6% <0%> (-13.05%)`	⬇️
tcms_api/config.py	`75.8% <0%> (-4.04%)`	⬇️
tcms/tests/factories.py	`82.55% <0%> (-3.03%)`	⬇️
tcms/testplans/views.py	`76.13% <0%> (-1.12%)`	⬇️
tcms/testruns/models.py	`69.72% <0%> (-0.77%)`	⬇️
tcms/core/admin.py	`100% <0%> (ø)`	⬆️
tcms/core/views/__init__.py	`100% <0%> (ø)`	⬆️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0760b04...5dd3883. Read the comment docs.

atodorov · 2018-03-07T10:32:02Z

tcms/core/ajax.py

@@ -225,6 +225,21 @@ def remove(self):
            tag = Tag.objects.get(name=self.tag)
            self.obj.remove_tag(tag)

+    class TagCounter:


Move this class out of here and add tests for it.

atodorov · 2018-03-07T10:32:34Z

tcms/core/ajax.py

+                except StopIteration:
+                    return 0
+
+            return self.counter[self.key] if tag.pk == self.counter['tag'] else 0


Better use explicit if-then-else construct

atodorov

This is starting to get better but still needs more work. There are also some missing tests:

When given a tag ID which doesn't match the current record from counter iterator,
e.g. trying to get the count for object which isn't inside the query
but the query isn't empty

No test simulating real example, e.g. counting several tags
which may be present inside Case, Plan, Run but not necessarily inside all of them.

Edge case is when counting several tags, e.g. 3 and tag1 is present for
Plan, Case and Run, tag2 only for Plan and Case, and tag3 only for Plan.

this will validate the counter class logic since it deals with iterators
and these iterators can be of different length.

A variation of the above is a test where we have more than 3 tags and they
are not present for all types of objects, thus the counting iterators are
not the same length. However every other tag happens to be assigned to
multiple objects. This will validate that we properly advance to the next
object inside the counting iterator. Indeed this will expose a bug in the current implementation.

atodorov · 2018-03-09T21:17:16Z

tcms/core/ajax.py

    context_data = {
        'tags': tags,
        'object': obj,
    }
    return render(request, template_name, context_data)


+class _TagCounter:
+    """ Used for counting the number of times a tags is assigned to TestRun/TestCase/TestPlan """


tags -> tag

While at it add doc-strings for both the init method and the calculate_tag_count method. See xmlrpc/api for examples how to format the description of parameters.

atodorov · 2018-03-09T21:19:21Z

tcms/core/tests/test_ajax.py

 from tcms.tests import BasePlanCase
+from tcms.core.ajax import _TagCounter


Move that above the imports of TestCase and TestCaseTag. Tests for the most part follow the convention to import classes and functions from the Python standard library first, then from Django and various other dependencies, then tcms and finally all testing related classes.

atodorov · 2018-03-09T21:20:09Z

tcms/core/tests/test_ajax.py

-from tcms.tests.factories import TestRunFactory
-from tcms.tests.factories import TestCaseFactory
-from tcms.tests.factories import TestPlanFactory
+from tcms.tests.factories import TagFactory, TestRunFactory, TestCaseFactory, TestPlanFactory


Please next time don't make formatting changes if not necessary.

atodorov · 2018-03-09T21:22:29Z

tcms/core/ajax.py

+
+        if tag.pk == self.counter['tag']:
+            return self.counter[self.key]
+        else:


Remove the else block and leave the return as the last return statement in the function.

FYI there's a pylint checker that will catch this.

atodorov · 2018-03-09T21:32:52Z

tcms/core/tests/test_ajax.py

+        self.assertEqual(count_for_tag_one, 3)
+        self.assertEqual(count_for_tag_two, 2)
+
+    def test_tag_count_empty_query(self):


Tip: if you are going to test only 1 class/main method as the test class name suggests then you can try naming the testing methods using test_description_of_conditions. This will help you avoid longer names and repeat the MUT name inside. In this case test_with_empty_query is shorter and more natural. Try doing this to see how it feels.

Tip: try using doc-strings with Given-When-Then or simply describe what do you expect this test to do and under what conditions.

In this particular example you are validating that the result is 0 when the QuerySet is empty. In other words code execution terminates after the StopIteration exception on line 274.

Is this what you really wanted ?

atodorov · 2018-03-09T21:34:55Z

tcms/core/tests/test_ajax.py

+
+    def test_tag_count_empty_query(self):
+        test_case_tags = TestCaseTag.objects.filter(
+            tag__in=[self.tag_one, self.tag_two]).values('tag').annotate(


In this case you may use tag=-1 or even objects.none() to save on some typing and multiline indentations.

If the query is empty it doesn't matter how much annotations you make, it is still going to be empty.

atodorov · 2018-03-09T21:49:55Z

tcms/core/ajax.py

+        self.counter = {'tag': 0}
+
+    def calculate_tag_count(self, tag):
+        if not self.counter['tag'] == tag.id:


use tag.pk instead of id

atodorov · 2018-03-09T22:00:29Z

For the record, I have no idea why coverage doesn't want to include the new class in its reports and this is driving me crazy.

atodorov · 2018-03-09T21:56:49Z

tcms/core/ajax.py

    context_data = {
        'tags': tags,
        'object': obj,
    }
    return render(request, template_name, context_data)


+class _TagCounter:


Inherits from (object). Again pylint will catch this I believe.

atodorov · 2018-03-12T21:33:52Z

tcms/core/tests/test_ajax.py

+        tag_one = TagFactory()
+        tag_two = TagFactory()
+        tag_three = TagFactory()
+        tags = [tag_one, tag_two, tag_three]


These are already created as class attributes in the setup above, why duplicate the setup ?

atodorov · 2018-03-12T21:44:37Z

tcms/core/ajax.py

+        :return: the number of times a tag is assigned to object
+        :rtype: int
+        """
+        if not self.counter['tag'] == tag.pk:


not == -> !=

Removed useless line from ajax.py

asankov force-pushed the ajax_sql_refactor branch from 1da30f4 to a2092c3 Compare February 24, 2018 00:08

asankov force-pushed the ajax_sql_refactor branch 12 times, most recently from 7e233c1 to 0f2e327 Compare March 1, 2018 22:20

atodorov requested changes Mar 7, 2018

View reviewed changes

asankov force-pushed the ajax_sql_refactor branch 3 times, most recently from 86a8b33 to 53a2f80 Compare March 7, 2018 15:23

atodorov requested changes Mar 9, 2018

View reviewed changes

asankov force-pushed the ajax_sql_refactor branch 6 times, most recently from 0803305 to c3ab772 Compare March 12, 2018 09:51

atodorov requested changes Mar 12, 2018

View reviewed changes

asankov force-pushed the ajax_sql_refactor branch from c3ab772 to 05a9726 Compare March 14, 2018 19:50

Refactor SQL queries in ajax.py to Django query

5dd3883

Removed useless line from ajax.py

asankov force-pushed the ajax_sql_refactor branch from 05a9726 to 5dd3883 Compare March 14, 2018 20:05

atodorov approved these changes Mar 14, 2018

View reviewed changes

atodorov merged commit 7cd0985 into kiwitcms:master Mar 14, 2018

asankov deleted the ajax_sql_refactor branch March 15, 2018 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor SQL queries in ajax.py to Django queries #232

Refactor SQL queries in ajax.py to Django queries #232

asankov commented Feb 23, 2018 •

edited

coveralls commented Feb 24, 2018 •

edited

codecov-io commented Feb 24, 2018 •

edited

atodorov Mar 7, 2018

atodorov Mar 7, 2018

atodorov left a comment •

edited

atodorov Mar 9, 2018

atodorov Mar 9, 2018

atodorov Mar 9, 2018

atodorov Mar 9, 2018

atodorov Mar 9, 2018

atodorov Mar 9, 2018

atodorov Mar 9, 2018

atodorov Mar 9, 2018

atodorov Mar 9, 2018

atodorov commented Mar 9, 2018

atodorov Mar 9, 2018

atodorov Mar 12, 2018

atodorov Mar 12, 2018

		from tcms.tests import BasePlanCase
		from tcms.core.ajax import _TagCounter

Refactor SQL queries in ajax.py to Django queries #232

Refactor SQL queries in ajax.py to Django queries #232

Conversation

asankov commented Feb 23, 2018 • edited

coveralls commented Feb 24, 2018 • edited

Pull Request Test Coverage Report for Build 749

💛 - Coveralls

codecov-io commented Feb 24, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atodorov left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atodorov commented Mar 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asankov commented Feb 23, 2018 •

edited

coveralls commented Feb 24, 2018 •

edited

codecov-io commented Feb 24, 2018 •

edited

atodorov left a comment •

edited