1474207 - Network Monitor response payload testing method variances

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[Brandon Cheng]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180704003137

Steps to reproduce:

At the moment, we have several different ways of testing the contents of CodeMirror in the "Response payload" panel of the Developer Tools Network Monitor.

1. Using CodeMirror.getValue(): https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/devtools/client/netmonitor/test/browser_net_cyrillic-02.js#53
2. Selecting ".CodeMirror-line" nodes and joining their textContent: https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/devtools/client/netmonitor/test/browser_net_post-data-01.js#139
3. Grabbing textContent directly from the container: https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/devtools/client/netmonitor/test/browser_net_post-data-02.js#45

Unfortunately, the DOM access based methods (2 & 3) are non-deterministic since CodeMirror renders its text virtually. The lines rendered to the DOM depend on the current scroll position and what's visible.

What's visible can change due changes unrelated to CodeMirror's container. For example:
- In bug 1353319, the default height of the CodeMirror container div was changed since an additional HTML preview panel was added.
- In bug 1428521, simply adding a horizontal scrollbar changed what lines CodeMirror chose to render to the DOM.
- If the content of the testing fixture/payload shifts around, a test may fail since CodeMirror may not render that line.

I think method 1 is the best since it grabs text that CodeMirror plans to render directly from memory. It works irrespective of the initial height of the developer tools, and where the tested payload is in the network response.

Here's its documentation: https://codemirror.net/doc/manual.html#getValue

I'd like to change all our network tests to use method 1, so changes unrelated to CodeMirror don't randomly fail. I'm a volunteer, so I won't start work on this until someone agrees with the above and confirms the bug.

Comment 1

[Brandon Cheng]

I should mention that I'm biased towards method 1 (CodeMirror.getValue) since I discovered it while working on a different bug. There may be problems with it I'm not seeing.

Comment 2

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

Thanks for the report and helping with the tests!

I agree that #1 seems to be the right way - especially because the official doc does clearly say that this method is used to get the current editor content. So, yes #2 should use that too.

However #3 is not related to code mirror. Check out the selector ".tree-section .treeLabel", it's a query for tree view component coming from DevTools code base: https://searchfox.org/mozilla-central/source/devtools/client/shared/components/tree
Post data are not rendered using code mirror. Btw. this part is not virtualized, so there should be no problem with initial size.

Anyway fixing #2 and avoid intermittent failures makes good sense to me!

Honza

Comment 3

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

Should I assign the bug to you?
Honza

Comment 4

[Brandon Cheng]

Yes please!

Comment 5

[Patrick Brosset <:pbro>]

This bug has not been updated in the last 6 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.

Comment 6

[Hemakshi Sachdev [:hemakshis]]

Hi!
I would like to take up this issue, I have worked on a couple of tests previously and from what I understand is that we need to change the way we are accessing the values rendered by the .CodeMirror component.
Could it be assigned to me?

Hemakshi

Comment 7

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

Assigned to you, thanks for the help Hemakshi!
Honza

Comment 8

[Hemakshi Sachdev [:hemakshis]]

Attachment: Bug 1474207 - Network Monitor response payload testing method variances. r=Honza

Changed the way we to access contents of .CodeMirror, by using CodeMirror.getValue() instead of .textContent

Comment 9

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

I am seeing some eslint errors
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a014db45b142fac16b29593dd22a1cc5dee8bb88&selectedJob=231901748

Honza

Comment 10

[Hemakshi Sachdev [:hemakshis]]

I have updated the diff with the requested changes

Comment 11

[Hemakshi Sachdev [:hemakshis]]

Hi @Honza,

I think now the patch is ready for review in the phabricator.

Hemakshi

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f32615dcb330
Network Monitor response payload testing method variances. r=Honza

Comment 13

[Brandon Cheng]

@hemakshis Thanks a ton for taking over on this.

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/f32615dcb330