Add count_skips field to Importance by Component widget #524
Conversation
| for component in sdatdict.keys(): | ||
| for bnum in sdatdict[component].keys(): | ||
| for importance in sdatdict[component][bnum].keys(): | ||
| total = 0 | ||
| passed = 0 | ||
| res_list = [] | ||
| for item in sdatdict[component][bnum][importance]: | ||
| total += 1 | ||
| if count_skips or (not item["result"] == "skipped"): |
There was a problem hiding this comment.
I don't believe this is quite the logic we want to apply here, but correct me if I'm interpreting something wrong here.
First, let's confirm what Count Skips means, in your description you reference the jenkins heatmap implementation for counting skipped results. In that case, the boolean controls whether a skip result counts as a pass, or as a failure. count_skips=true means include skip results when subtracting from the total to get the pass count. For a run with skip results, when count_skips goes from false to true the pass percentage will drop, as passes = total - (fail + error + xpass + xfail + skip). The total count of tests stays the same, but the pass percentage changes as now failures are counting skips.
For this set of importance+component filtered results, you want to control whether or not skips are counted as passes in the total result.
Taking the default behavior into account, when count_skips is false and a skipped item is in the result:
count_skips=False and item["result"] == "skipped"
The total count will not increase, which changes the default behavior of this widget's pass percentage calculation. Given this current logic, count_skips default should be True to keep the behavior of the widget consistent with the current calculations.
That aside, let's look at what happens next.
again, count_skips=False and item["result"] == "skipped"
The passed count doesn't change, the total is just lower.
I think you should be concerned with modifying passed and the calculation of percentage in sdatdict on L109/111 below, instead of trying to modify the total.
Use the boolean to reflect whether skip results are counted as failures, and mirror the logic of the jenkins_heatmap widget by calculating passes not just by pass but also by including xfail and xpass as failures too.
This way you'll be addressing all three of the states from the TODO, instead of just one of the three.
There was a problem hiding this comment.
Thanks for the review and the logic validation here.
I added a defaultdict to store the count of every different result type. Therefore, a more verbose sum can be implemented, this mimics better the logic of the heatmap widget
Fynardo
force-pushed
the
importance_count_skips
branch
from
08f6e73
to
f0a1054
Compare
| @@ -91,20 +94,33 @@ def get_importance_component( | |||
| sdatdict[component][bnum][importance] = [] | |||
|
|
|||
| # this is to change result values into numbers | |||
| # TODO: This doesn't handle xpassed, xfailed, skipped, etc. so figure that out | |||
| for component in sdatdict.keys(): | ||
| for bnum in sdatdict[component].keys(): | ||
| for importance in sdatdict[component][bnum].keys(): | ||
| results_dict = defaultdict(int) |
There was a problem hiding this comment.
Looking good locally with a few advisor job result archives loaded.
The only thing that's missing at this point from both widgets is support of manual result differentiation for skipping. Right now these count as passed, and this PR doesn't change that.
I think in followup PRs we can define support for additional control of account for manual, skipped, and blocked results uniquely within the widgets.
|
@Fynardo I'll include this and your other PR along with frontend updates for 2.5.11, and will ask you to come back and help validate the frontend updates in stage where we have a large amount of test results loaded to filter against. |
|
@mshriver Count me in for further testing (my local DB is also just half-dozen runs) and to enhance support for more results |
As done in other widgets, like Jenkins heatmap, it would be useful to allow users to decide if they want to count skipped tests or not in Importance by Component widget.