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

Make testcase filtering on the API side aware of the duration fields #2530

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 46 additions & 2 deletions tcms/rpc/api/testcase.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
# -*- coding: utf-8 -*-

from datetime import timedelta

from django.db.models import Case, F, Q, When
from django.forms import EmailField, ValidationError
from django.forms.models import model_to_dict
from django.utils.dateparse import parse_duration
from modernrpc.core import REQUEST_KEY, rpc_method

from tcms.core import helpers
from tcms.core.utils import form_errors_to_list
from tcms.management.models import Component, Tag
from tcms.rpc import utils
from tcms.rpc.api.forms.testcase import NewForm, UpdateForm
from tcms.rpc.api.utils import stringify_values
from tcms.rpc.decorators import permissions_required
from tcms.testcases.models import TestCase, TestCasePlan

Expand Down Expand Up @@ -279,8 +284,37 @@ def filter(query=None): # pylint: disable=redefined-builtin
if query is None:
query = {}

return list(
TestCase.objects.filter(**query)
for key, val in query.items():
if not key.startswith(
("setup_duration", "testing_duration", "expected_duration")
):
continue

try:
duration = parse_duration(val)
except TypeError:
# val isn't a string or byte-like object
# item is probably something like 'setup_duration__isnull=True'
continue

if duration is None:
continue

query[key] = duration
Copy link
Contributor Author

@mfonism mfonism Sep 16, 2021

Choose a reason for hiding this comment

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

I added this for loop because the values of the duration keys in the incoming dict query will always be strings as xmlrpc can't marshal them if they are datetime.duration objects. The loop parses these strings (if they are present) into datetime.duration objects -- so that QuerySet.filter() won't choke on them.

Copy link
Contributor Author

@mfonism mfonism Sep 16, 2021

Choose a reason for hiding this comment

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

Since we don't need to filter by duration fields on the API side, then I'll take this out.

Although if someone ever mistakenly sends in a lookup based on a duration field, it will choke.


qs = (
TestCase.objects.annotate(
expected_duration=Case(
When(
Q(setup_duration__isnull=True) & Q(testing_duration__isnull=True),
then=timedelta(0),
),
When(Q(setup_duration__isnull=True), then="testing_duration"),
When(Q(testing_duration__isnull=True), then="setup_duration"),
default=F("setup_duration") + F("testing_duration"),
)
)
.filter(**query)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like there may be some confusion around this item. Being able to filter by the duration field is the last checklist item in #1923. That should not need special casing for setup_duration and testing_duration because these are model fields. The only special case is expected_duration.

OTOH the item "API method TestCase.filter returns the value of all 3 fields - tests are updated to match" refers only to making sure that the API method TestCase.filter includes the new fields in its results. That should be much easier and require a lot less changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh oh. I was over thinking it.

I thought the current checklist item also meant that filtering with respect to the duration fields should be possible on the API side.

I will create a new PR to implement just the presence of the duration fields in the result.

I'll keep this one alive (closed) so one can refer to it if we ever need to add filtering with respect to the duration fields on the API side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just occured to me that I can shorten the entire Case expression with Coalesce:

Coalesce("setup_duration", timedelta(0)) + Coalesce("testing_duration", timedelta(0))

.values(
"id",
"create_date",
Expand All @@ -304,9 +338,19 @@ def filter(query=None): # pylint: disable=redefined-builtin
"default_tester__username",
"reviewer",
"reviewer__username",
"setup_duration",
"testing_duration",
"expected_duration",
)
.distinct()
)
return [
stringify_values(
testcase_dict,
keys=["setup_duration", "testing_duration", "expected_duration"],
)
for testcase_dict in qs.iterator()
]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I loop through the list of dicts (representing testcases) and make sure that if the value of the duration keys are not null, they are converted from duration objects to strings as xmlrpc cannot marshal duration objects.


@permissions_required("testcases.view_testcase")
Expand Down
8 changes: 8 additions & 0 deletions tcms/rpc/api/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,11 @@ def tracker_from_url(url, request):
return import_string(bug_system.tracker_type)(bug_system, request)

return None


def stringify_values(d, keys=None):
keys = keys or []
return {
key: (str(val) if (key in keys and val is not None) else val)
for (key, val) in d.items()
}
24 changes: 24 additions & 0 deletions tcms/rpc/tests/test_testcase.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,36 @@ def test_filter_query_none(self):
self.assertIn("author", result[0])
self.assertIn("default_tester", result[0])
self.assertIn("reviewer", result[0])
self.assertIn("setup_duration", result[0])
self.assertIn("testing_duration", result[0])
self.assertIn("expected_duration", result[0])

def test_filter_by_product_id(self):
cases = self.rpc_client.TestCase.filter({"category__product": self.product.pk})
self.assertIsNotNone(cases)
self.assertEqual(len(cases), self.cases_count)

def test_filter_by_setup_duration(self):
TestCaseFactory(setup_duration=timedelta(seconds=30))
cases = self.rpc_client.TestCase.filter({"setup_duration": "00:00:30"})
self.assertIsNotNone(cases)
self.assertEqual(len(cases), 1)

def test_filter_by_testing_duration(self):
TestCaseFactory(testing_duration=timedelta(minutes=5, seconds=1))
cases = self.rpc_client.TestCase.filter({"testing_duration__gt": "00:05:00"})
self.assertIsNotNone(cases)
self.assertEqual(len(cases), 1)

def test_filter_by_expected_duration(self):
TestCaseFactory(
setup_duration=timedelta(seconds=30),
testing_duration=timedelta(minutes=5, seconds=1),
)
cases = self.rpc_client.TestCase.filter({"expected_duration__lt": "00:5:31"})
self.assertIsNotNone(cases)
self.assertEqual(len(cases), self.cases_count)


class TestUpdate(APITestCase):
non_existing_username = "FakeUsername"
Expand Down