1432865 - Clean up show and hideColumn test API

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Netmonitor's tests are using two helpers:

* showColumn
* hideColumn

These are defined in several tests and also in head.js file

The bug should:
1) Make sure there is just one definition of these methods (if possible)
2) The methods use async/await instead of function*/yield

Honza

Comment 1

[Abhinav Koppula]

Attachment: Bug 1432865 - Clean up show and hideColumn test API.

Review commit: https://reviewboard.mozilla.org/r/215540/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215540/

Comment 2

[Abhinav Koppula]

Hi Honza,
I have created a review-request for this issue.

On searching showColumn in searchFox,
https://dxr.mozilla.org/mozilla-central/search?q=showColumn&redirect=false
showColumn in netmonitor exists only in devtools/client/netmonitor/test/browser_net_columns_pref.js

There is a method showColumn also present in storage/head.js. I don't think we are supposed to touch that, right?

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks!

I can't apply the patch on latest m-c head. Does this need to be rebased?

Honza

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Note that this is a follow up for bug 1431912

Honza

Comment 5

[Abhinav Koppula]

Comment on attachment 8945332 [details]
Bug 1432865 - Clean up show and hideColumn test API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215540/diff/1-2/

Comment 6

[Abhinav Koppula]

Hi Honza,
Have rebased with the latest m-c.

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8945332 [details]
Bug 1432865 - Clean up show and hideColumn test API.

https://reviewboard.mozilla.org/r/215540/#review221274

Thanks!

R+, but I have one inline question

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1beb471ef4f77207f48415bf8b5a0fa8a92640f

Honza

::: devtools/client/netmonitor/test/browser_net_columns_time.js:19
(Diff revision 2)
>    let Actions = windowRequire("devtools/client/netmonitor/src/actions/index");
>    store.dispatch(Actions.batchEnable(false));
>  
> -  hideColumn(monitor, "waterfall");
> -  showColumn(monitor, "endTime");
> -  showColumn(monitor, "responseTime");
> +  let visibleColumns = store.getState().ui.columns;
> +  if (visibleColumns.waterfall) {
> +    await hideColumn(monitor, "waterfall");

Why is this needed? The Waterfall column should be visible by default, no?

Here is the default set of visible columns:
https://searchfox.org/mozilla-central/rev/4611b9541894e90a421debb57ddbbcff55c2f369/devtools/client/netmonitor/test/head.js#99

Comment 8

[Abhinav Koppula]

Comment on attachment 8945332 [details]
Bug 1432865 - Clean up show and hideColumn test API.

https://reviewboard.mozilla.org/r/215540/#review221274

> Why is this needed? The Waterfall column should be visible by default, no?
> 
> Here is the default set of visible columns:
> https://searchfox.org/mozilla-central/rev/4611b9541894e90a421debb57ddbbcff55c2f369/devtools/client/netmonitor/test/head.js#99

Hi Honza,
Here, previously we were calling hideColumn and showColumn without yield. If I `await` for hideColumn/showColumn after my changes, the tests are failing because I think we are trying to toggle a column which is already visible. So instead, I check if the column is already visible/invisible and then call hideColumn/showColumn accordingly (like it is done for test - browser_net_columns_last_column.js) 

I am not sure why we were hiding/showing the above columns in the first place because even if I just remove those 5 statements, the test still passes.

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Abhinav Koppula from comment #8)
> Comment on attachment 8945332 [details]
> Bug 1432865 - Clean up show and hideColumn test API.
> 
> https://reviewboard.mozilla.org/r/215540/#review221274
> 
> > Why is this needed? The Waterfall column should be visible by default, no?
> > 
> > Here is the default set of visible columns:
> > https://searchfox.org/mozilla-central/rev/4611b9541894e90a421debb57ddbbcff55c2f369/devtools/client/netmonitor/test/head.js#99
> 
> Hi Honza,
> Here, previously we were calling hideColumn and showColumn without yield. If
> I `await` for hideColumn/showColumn after my changes, the tests are failing
> because I think we are trying to toggle a column which is already visible.
> So instead, I check if the column is already visible/invisible and then call
> hideColumn/showColumn accordingly (like it is done for test -
> browser_net_columns_last_column.js) 
OK
 
> I am not sure why we were hiding/showing the above columns in the first
> place because even if I just remove those 5 statements, the test still
> passes.
Ah, a comment would be helpful here.

Can you please yet append the following comment before hiding the waterfall column:

// Hide the waterfall column to make sure timing data are fetched
// by the other timing columns ("endTime", "responseTime", "duration",
// "latency").
// Note that all these timing columns are based on the same
// `RequestListColumnTime` component.

Honza

Comment 10

[Abhinav Koppula]

Comment on attachment 8945332 [details]
Bug 1432865 - Clean up show and hideColumn test API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215540/diff/2-3/

Comment 11

[Abhinav Koppula]

Hi Honza,
I've updated the review-request with the comment.

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

ok, thanks!

This is ready to land.
Honza

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41237a18fbea
Clean up show and hideColumn test API. r=Honza

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/41237a18fbea