658572 - Change test results to show only failures by default

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Result Dashboard, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Update the test results for all test reports to only show failures by default. Users will be able to see other results by changing the applied filter.

Comment 1

[u415141]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/13643363

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Set the default filter to failures. v1.0

I also took this opportunity to clean up a bit of the filter code and styling. When applying filters the fade in/out has been replaced with a simple show/hide, and the applied filter link is now bold rather than hidden.

Finally, I also added a message for when the applied filter has no results. This looks a bit better when there are no failures for example, as it could previously look like the results were unintentionally empty.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 534182 [details] [diff] [review]
Set the default filter to failures. v1.0

>+      if ($('#result tbody tr:visible').length == 0) {

Please use a triple operator.

>+        $('#result tbody').append('<tr id="noresults"><td colspan="' + $('#result tr th').length + '">No results match the current filter.</td></tr>');

Please try to keep the 80 characters by moving out code to separate lines.

>           $("#all").click(function (event) {
[..]
>           $("#passed").click(function (event) {
[..]
>           $("#failed").click(function (event) {
[..]
>           $("#skipped").click(function (event) {
[..]
>+            filter_report("#skipped", "#result tr.skipped");

Could we even set the callback inside the helper function? The code looks quite similar in all those cases and would make it even more cleaner.

r=me with the changes above applied.

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Set the default filter to failures. v1.1

(In reply to comment #3)
> Comment on attachment 534182 [details] [diff] [review] [review]
> Set the default filter to failures. v1.0
> 
> >+      if ($('#result tbody tr:visible').length == 0) {

Done.

> Please use a triple operator.
> 
> >+        $('#result tbody').append('<tr id="noresults"><td colspan="' + $('#result tr th').length + '">No results match the current filter.</td></tr>');
> 
> Please try to keep the 80 characters by moving out code to separate lines.

Done.

> >           $("#all").click(function (event) {
> [..]
> >           $("#passed").click(function (event) {
> [..]
> >           $("#failed").click(function (event) {
> [..]
> >           $("#skipped").click(function (event) {
> [..]
> >+            filter_report("#skipped", "#result tr.skipped");
> 
> Could we even set the callback inside the helper function? The code looks
> quite similar in all those cases and would make it even more cleaner.

Sorry, I'm struggling to understand what you're meaning here. I have updated the patch to include less duplication by having one method to set the filter callbacks and another to apply the filters. I can't see how I'd combine these into one method.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 534454 [details] [diff] [review]
Set the default filter to failures. v1.1

>+    var set_filters = function() {

Please use named functions, similar to what we are doing now in Mozmill tests.

>+      $("#all").click(function (event) {
>+        apply_filter("#all", "#result tbody tr");
>+        event.preventDefault();
>+      });
>+
>+      $("#passed").click(function (event) {
>+        apply_filter("#passed", "#result tr.passed");
>+        event.preventDefault();
>+      });
>+
>+      $("#failed").click(function (event) {
>+        apply_filter("#failed", "#result tr.failed");
>+        event.preventDefault();
>+      });

Built-up an array with all possible filter values including 'all'. Iterate over all of them (via forEach) and execute a callback (which is now apply_filter()) which simply constructs the right CSS values. For the 'all' case you can remove the 'tbody' element, which makes it more consistent.

I hope that will help you to understand what I want to see. For now I will set it to r-.

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Set the default filter to failures. v1.2

(In reply to comment #5)
> Comment on attachment 534454 [details] [diff] [review] [review]
> Set the default filter to failures. v1.1
> 
> >+    var set_filters = function() {
> 
> Please use named functions, similar to what we are doing now in Mozmill
> tests.

Done.

> >+      $("#all").click(function (event) {
> >+        apply_filter("#all", "#result tbody tr");
> >+        event.preventDefault();
> >+      });
> >+
> >+      $("#passed").click(function (event) {
> >+        apply_filter("#passed", "#result tr.passed");
> >+        event.preventDefault();
> >+      });
> >+
> >+      $("#failed").click(function (event) {
> >+        apply_filter("#failed", "#result tr.failed");
> >+        event.preventDefault();
> >+      });
> 
> Built-up an array with all possible filter values including 'all'. Iterate
> over all of them (via forEach) and execute a callback (which is now
> apply_filter()) which simply constructs the right CSS values. For the 'all'
> case you can remove the 'tbody' element, which makes it more consistent.

I think I finally understood. Sorry for being slow. :) Let me know if the updated patch is what you had in mind.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 534938 [details] [diff] [review]
Set the default filter to failures. v1.2

That looks great and makes it way better readable.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed as:
https://github.com/whimboo/mozmill-dashboard/commit/fcbed688ee8441c9ed9d3a8d78a04ba5276f8ac8

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

All changes have been pushed to production now.