-
-
Notifications
You must be signed in to change notification settings - Fork 296
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
Render Individual test case health on test case page #2277
base: master
Are you sure you want to change the base?
Conversation
acdab9b
to
948046c
Compare
tcms/telemetry/api.py
Outdated
negative += 1 | ||
all_count += 1 | ||
|
||
if test_execution.run.plan_id != plan_id: |
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.
Aggregating by plan_id only works in the scenario where we filter by case_id and query information for a single test case. However this doesn't work for the Test Run page where we'd like to query for multiple cases (even filter by run_id directly instead of the case_id) and where information needs to be organized by case_id or TE.id.
Maybe something like:
>>> for obj in TestExecution.objects.filter().values('run__plan', 'case_id', 'status__name', 'status__weight').order_by('case', 'run__plan', 'status__weight'):
... print(obj)
...
{'run__plan': 1, 'case_id': 1, 'status__name': 'ERROR', 'status__weight': -20}
{'run__plan': 1, 'case_id': 1, 'status__name': 'RUNNING', 'status__weight': 0}
{'run__plan': 1, 'case_id': 1, 'status__name': 'WAIVED', 'status__weight': 10}
{'run__plan': 1, 'case_id': 1, 'status__name': 'PASSED', 'status__weight': 20}
{'run__plan': 2, 'case_id': 1, 'status__name': 'PASSED', 'status__weight': 20}
{'run__plan': 1, 'case_id': 2, 'status__name': 'BLOCKED', 'status__weight': -10}
{'run__plan': 1, 'case_id': 2, 'status__name': 'RUNNING', 'status__weight': 0}
would be better on the API layer + an additional pre-processing layer on the FE depending on how we want to consume this information.
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.
@atodorov I pushed the changes to the API. however, I think the API is too generic this way, and we would need to do a lot of processing on the FE side. A better idea might be to have multiple APIs for the multiple use-cases of this feature, e.g. Testing.individual_test_case_health.test_run
and Testing.individual_test_case_health.test_case
for the different pages
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.
Indeed, the FE will need more processing. I think I've got a nice way to implement this into 1 API call.
For the TestCase page the query will be:
Select from TE, Where case_id=X, plan_id IN [...], order & aggregate statuses by plan_id,
where case_id is a scalar, plan_id is a list of IDs. The FE code on the TestCase page needs data[plan_id]['completion_rate | failure_rate']
For the TestRun page the query will be:
Select from TE, Where run_id=X, case_id IN[...], order & aggregate statuses by case_id
where run_id is a scalar, case_id is a list of IDs. The FE code here will need data[case_id]['completion_rate | failure_rate'].
I think if you modify the signature and accept 2 parameters while the 2nd one also controls the list of returned values and how they are aggregated we can have 1 generic function which will be consumed in 2 different places.
The inner workings of the function will not change but how it groups the results can be flexible depending on what we need.
99335ab
to
f2bd8f3
Compare
f2bd8f3
to
34c70cc
Compare
697fd66
to
349e020
Compare
@atodorov PTAL. since they last time I worked on this was few months ago I lost context. I made the API as simple as possible and transfered all counting logic on the UI side + I wrote a ugly minimalistic UI so that we have a notion of how the data is going to be used. let's start with this and as discuss the necessary changes to make the API a bit less generic, but still reusable between the TC and TP page |
6d2a296
to
4d60aca
Compare
@atodorov do you need more input for me on this, or should I just wait for you to get to it? I have started working on the UI for the test run page and I can push that as well, if you want |
Push whatever you have and I will review as soon as I can |
<div class="progress-bar progress-bar-danger" role="progressbar"></div> | ||
<div class="progress-bar progress-bar-success" role="progressbar"></div> | ||
</div> | ||
</div> |
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 a missing div tag somewhere around here.
I'm not sure what your plan for the UI is but I'd prefer that this progress bar be consistent with other and display the negative/positive parts in a single line.
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.
To clarify, see the progress bar at the top of Execution trends and as the data shows we've got 3 states here negative/neutral/positive.
4d60aca
to
ec62206
Compare
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.
With the latest commit I am still seeing the same as in #2277 (comment) which is a bug. The progress bars are totally different than the actual counts.
the last commit should fix the calculation logic. let's discuss the UI after this is resolved. I think it makes sense to make the UI similar to the one in Execution trends |
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.
Last commit introduces even more errors and makes things worse. Next time test before pushing.
@@ -28,23 +28,24 @@ <h2 class="card-pf-title"> | |||
<a href="{% url 'test_plan_url_short' execution.run.plan.pk %}">TP-{{ execution.run.plan.pk }}: {{ execution.run.plan.name }}</a> | |||
</div> | |||
<div class="list-group-item-text"> | |||
<div class="completion-rate-container">{% trans 'Completion rate' %}: <span class="completion-rate"></span> | |||
<div class="completion-rate-container">{% trans 'Completion rate' %}: <span class="completion-rate"></div> |
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.
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.
it should work OK now. however, I don't think we should make this look like Execution trends. In Execution trends we have 3 types of statuses, that depend on one another, e.g. the more FAILs we have, the less SUCCESS we have.
Here, we have 2 metrics that are independent, we could have 100% completion rate and 100% failure rate, but we could also have 100% completion rate and 0% failure rate.
9bd4cdc
to
899e174
Compare
First commit without UI patches cherry-picked as part of #2537 |
8f9663b
to
337304b
Compare
337304b
to
f94cd6a
Compare
Testing.individual_test_case_health
APIThere 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.
This will be used to render individual test case health widget