Closed Bug 1474207 Opened Last year Closed 6 months ago

Network Monitor response payload testing method variances

Categories

(DevTools :: Netmonitor, defect, P2)

61 Branch
defect

Tracking

(firefox67 fixed)

RESOLVED FIXED
Firefox 67
Tracking Status
firefox67 --- fixed

People

(Reporter: brandon.cheng, Assigned: hemakshis)

Details

Attachments

(1 file)

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.
Component: Untriaged → Netmonitor
Product: Firefox → DevTools
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.
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Should I assign the bug to you?
Honza
Flags: needinfo?(brandon.cheng)
Yes please!
Flags: needinfo?(brandon.cheng)
Assignee: nobody → brandon.cheng
Status: NEW → ASSIGNED

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.

Assignee: brandon.cheng → nobody
Status: ASSIGNED → NEW

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

Flags: needinfo?(odvarko)

Assigned to you, thanks for the help Hemakshi!
Honza

Assignee: nobody → sachdev.hemakshi
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)

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

I have updated the diff with the requested changes

Flags: needinfo?(sachdev.hemakshi)

Hi @Honza,

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

Hemakshi

Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f32615dcb330
Network Monitor response payload testing method variances. r=Honza

@hemakshis Thanks a ton for taking over on this.

Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
You need to log in before you can comment on or make changes to this bug.