Bug 202106 – results.webkit.org: Add investigation drawer

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

Add drawer to allow investigation of a specific data point.

[Jonathan Bedard]

Created attachment 379378 [details]
Patch

Comment on attachment 379378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379378&action=review

> Tools/resultsdbpy/resultsdbpy/view/static/js/drawer.js:71
>              let candidates = document.getElementsByClassName("main");
> -            if (candidates.length)
> -                main = candidates[0];
> +            mains = [];
> +            for (let count = 0; count < candidates.length; ++count)
> +                mains.push(candidates[count]);

Is there any reason to have mains and candidates separately? They will be end up with the same content, aren't they?

> Tools/resultsdbpy/resultsdbpy/view/static/js/drawer.js:72
> +            console.log(mains);

Do wen need the logging here?

> Tools/resultsdbpy/resultsdbpy/view/static/js/expectations.js:38
> +    }

Nit: do we expect one blank line between each function?

> Tools/resultsdbpy/resultsdbpy/view/static/js/expectations.js:48
> +    }

ditto

> Tools/resultsdbpy/resultsdbpy/view/static/js/expectations.js:51
> +    }

ditto

> Tools/resultsdbpy/resultsdbpy/view/static/js/investigate.js:32
> +            let params = {

Use `const` whenever we can?

> Tools/resultsdbpy/resultsdbpy/view/static/js/investigate.js:74
> +            result += ' minutes to run';

Do we need leading space here as result before this statement should have tailing space.

> Tools/resultsdbpy/resultsdbpy/view/static/js/investigate.js:88
> +    const result = Math.ceil(value / max * 100 - .5);

Isn't this equivalent to const result = Math.floor(value / max * 100);

> Tools/resultsdbpy/resultsdbpy/view/static/js/investigate.js:98
> +    let result = [];

This can be a `const`.

[Jonathan Bedard]

Comment on attachment 379378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379378&action=review

>> Tools/resultsdbpy/resultsdbpy/view/static/js/drawer.js:71
>> +                mains.push(candidates[count]);
> 
> Is there any reason to have mains and candidates separately? They will be end up with the same content, aren't they?

Because getEleementsByClassName doesn't actually return an array, it returns something like an array. So we couldn't use forEach. Don't feel very strongly about it, but that's why I did it.

>> Tools/resultsdbpy/resultsdbpy/view/static/js/drawer.js:72
>> +            console.log(mains);
> 
> Do wen need the logging here?

No, that should be removed.

>> Tools/resultsdbpy/resultsdbpy/view/static/js/expectations.js:38
>> +    }
> 
> Nit: do we expect one blank line between each function?

Not sure....oddly, we don't have strict style guidelines for JavaScript (or at least, the aren't enforced with check-webkit-style)

[Zhifei Fang]

Comment on attachment 379378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379378&action=review

>> Tools/resultsdbpy/resultsdbpy/view/static/js/investigate.js:88
>> +    const result = Math.ceil(value / max * 100 - .5);
> 
> Isn't this equivalent to const result = Math.floor(value / max * 100);

95.4 =>  94.9 => 95
95.6 => 95.1 => 96 

This is more like Math.round

[Zhifei Fang]

Comment on attachment 379378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379378&action=review

> Tools/resultsdbpy/resultsdbpy/view/static/js/investigate.js:65
> +        const time = data.stats.end_time - data.stats.start_time;

This looks like a time stamp, you can directly use new Date(time) to get the minutes and seconds directly https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date

> Tools/resultsdbpy/resultsdbpy/view/static/js/investigate.js:131
> +                    ${data.start_time ? succeeded.toLocaleString() : percentage(succeeded, data.stats.tests_run)} passed

Display actual `${succeeded} / ${data.stats.tests_run} ${percentage(succeeded, data.stats.tests_run)}` may be more clear

> Tools/resultsdbpy/resultsdbpy/view/static/js/investigate.js:147
> +                ${data.start_time ? value.toLocaleString() : percentage(value, data.stats.tests_run)} ${type}

Ditto

> Tools/resultsdbpy/resultsdbpy/view/static/js/investigate.js:169
> +        `${data.length} reports for ${agregateData.configuration}`,

If we have multiple test results for the same configuration, will the data always be the same, like how many tests run, how many tests failed? I think list the detail of each test result can help the user to understand the percentage you calculated. And it will be more friendly for user to look into the details.

> Tools/resultsdbpy/resultsdbpy/view/static/js/investigate.js:203
> +        const result = this.ref && this.content && this.close && this.previous && this.next;

Add event streams for expand / collapse / select can help you to easily access the ref.element, the check here won't grantee you have the ref.element property because ref will only has the element when the element mounted on the DOM, just calling REF.createRef() won't have any dom element inside of it.

For event stream you can add action handler in those refs' onElementMount, thus all those handlers can be granted to have the ref's element. You will need to unregister those handlers when onElementUnmounted to make sure those won't triggered for destroyed ref.

> Tools/resultsdbpy/resultsdbpy/view/static/js/investigate.js:234
> +        return `<div class="drawer main right-sidebar" ref="${this.ref}" style="z-index: 20">

I'd love to have a list of results on the left of the drawer.

> Tools/resultsdbpy/resultsdbpy/view/static/js/investigate.js:240
> +                        <a class="button" style="cursor: pointer" ref="${this.close}">Close</a>

You may want to use the "close-header" see my dashboard for example.

[Jonathan Bedard]

Comment on attachment 379378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379378&action=review

>> Tools/resultsdbpy/resultsdbpy/view/static/js/investigate.js:131
>> +                    ${data.start_time ? succeeded.toLocaleString() : percentage(succeeded, data.stats.tests_run)} passed
> 
> Display actual `${succeeded} / ${data.stats.tests_run} ${percentage(succeeded, data.stats.tests_run)}` may be more clear

That gets pretty long and unwieldy with hundreds of thousands of results reporting.

>> Tools/resultsdbpy/resultsdbpy/view/static/js/investigate.js:169
>> +        `${data.length} reports for ${agregateData.configuration}`,
> 
> If we have multiple test results for the same configuration, will the data always be the same, like how many tests run, how many tests failed? I think list the detail of each test result can help the user to understand the percentage you calculated. And it will be more friendly for user to look into the details.

Not necessarily. You can inspect existing results to confirm that.

I think a list of each configuration with the a bubble for it's state would go a long ways to helping, though.

>> Tools/resultsdbpy/resultsdbpy/view/static/js/investigate.js:240
>> +                        <a class="button" style="cursor: pointer" ref="${this.close}">Close</a>
> 
> You may want to use the "close-header" see my dashboard for example.

I did look at this....I'd like to find a way to integrate the close header with the next/previous buttons, otherwise we'd have a 2-tiered header, and don't think we have enough space.

[Jonathan Bedard]

Created attachment 379514 [details]
Patch

[Zhifei Fang]

unofficially r=me

[Jonathan Bedard]

Created attachment 379559 [details]
Patch

[Aakash Jain]

rs=me

r=me

[WebKit Commit Bot]

Comment on attachment 379559 [details]
Patch

Clearing flags on attachment: 379559

Committed r250355: <https://trac.webkit.org/changeset/250355>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55716341>