1589011 - Use an iframe wrapper in existing DAMP tests

Status: RESOLVED FIXED

(DevTools :: General, task, P1)

Description

[Julian Descottes [:jdescottes]]

The goal of this bug is to modify existing DAMP tests to have one iframe wrapping the test document.

This is more or less what was already done in https://phabricator.services.mozilla.com/D48500, except that we should make sure the inspector can transparently use a setup with an iframe.

I would like to keep Bug 1454025 for adding new tests that will use many iframes and focus on features that need to fetch resources on all those frames (inspector search? project search?).

The goal of this bug is only to add a single iframe, for now same origin. The idea is that this should provide some mild coverage of Fission-related regressions, even though we can't be sure it will be relevant.

Comment 1

[Julian Descottes [:jdescottes]]

Before I go back to this I would like to have a clear agreement on the plan. I had a first one in https://bugzilla.mozilla.org/show_bug.cgi?id=1454025#c8 but I didn't explicitly ask for feedback then so let's do it here.

We have three basic test suites we can modify: simple, complicated, custom.
In the previous reviews/discussions in Bug 1454025, we gravitated towards only applying this to custom. I would like to discuss this again, because custom is only running against 3 tools: inspector, debugger, console. While simple/complicated also cover netmonitor and styleeditor.

My initial proposal was to keep simple without iframes, but have complicated and custom with iframes. The idea was to keep simple free of iframes to have a "non-fission" baseline, but migrate everything else. There was a pushback on this approach because the inspector test was too impacted by the iframe wrapper. So we decided to only do it for custom, but again the inspector here needed a special treatment.

First what should we do about the inspector.
I can modify the inspector test and the inspector behavior so that the added "iframe" doesn't make the open & reload tests irrelevant.
For open we need to use "inspectNode" in order to open the inspector on an element in the iframe. For reload we need to fix the logic that reselects the previously selected node, so that it works across iframes. Note that this means we are no longer testing the same interactions. Before we used to test just open, now we test open + findNodeFront/setNodeFront etc... so we should expect the baseline to slightly modified.
I think the "Inspect Element" interaction is very important, so I think the change is fine, but we just need to be aware about this.

Then what should we do about the test suites.
Provided the inspector tests can run with the iframe, I would be in favor of migrating both complicated & custom tests to iframes, so that we don't miss the netmonitor.

Alex, can you take a look at this plan, and say if you agree or disagree? Thanks.

Comment 2

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes] from comment #1)

Before I go back to this I would like to have a clear agreement on the plan. I had a first one in https://bugzilla.mozilla.org/show_bug.cgi?id=1454025#c8 but I didn't explicitly ask for feedback then so let's do it here.

We have three basic test suites we can modify: simple, complicated, custom.
In the previous reviews/discussions in Bug 1454025, we gravitated towards only applying this to custom. I would like to discuss this again, because custom is only running against 3 tools: inspector, debugger, console. While simple/complicated also cover netmonitor and styleeditor.

My initial proposal was to keep simple without iframes, but have complicated and custom with iframes. The idea was to keep simple free of iframes to have a "non-fission" baseline, but migrate everything else. There was a pushback on this approach because the inspector test was too impacted by the iframe wrapper. So we decided to only do it for custom, but again the inspector here needed a special treatment.

My main argument against doing it for complicated was about having only the inspector to use a distinct setup from any other panel.
All panels would use bild.de copy within an iframe, except the inspector.
I would really expect simple and complicated to be testing against the exact same, for all the panels:

• a blank document
• a copy of bild.de (or whatever website we care about)

First what should we do about the inspector.
I can modify the inspector test and the inspector behavior so that the added "iframe" doesn't make the open & reload tests irrelevant.
For open we need to use "inspectNode" in order to open the inspector on an element in the iframe. For reload we need to fix the logic that reselects the previously selected node, so that it works across iframes. Note that this means we are no longer testing the same interactions. Before we used to test just open, now we test open + findNodeFront/setNodeFront etc... so we should expect the baseline to slightly modified.
I think the "Inspect Element" interaction is very important, so I think the change is fine, but we just need to be aware about this.

Sounds like a good idea. It is fine modifying the baseline as soon as it is justified and that we record it as a test change.
The DevTools perf dashboard allows to ignore such change.
On top of that, it sounds great to cover these two new usecase: inspect element and reload with a selected element!

Then what should we do about the test suites.
Provided the inspector tests can run with the iframe, I would be in favor of migrating both complicated & custom tests to iframes, so that we don't miss the netmonitor.

That, I don't know. I'm wondering if it would still be helpful to have three level of testing against the inspector:

• simple => blank document, almost empty markup view and sidebars
• complicated => average document, average markup view, no particular interaction
• custom => stress test doc, stressed markup view and sidebars, particular interactions

If the baseline of custom.open or reload change significantly with your changes, we might loose coverage of smaller regressions which may not impact "simple".
What about doing it for custom and revisit the decision against complicated based on DAMP results?

Note that netmonitor really ought to have its own custom test.
And styleeditor tests may be completely wrong. If I recall correctly we aren't waiting for anything corrrectly for this panel, so we most likely only measure the time it takes to load the document and not to update the panel. Opening may also be leaking many async operations into reload...

Comment 3

[Julian Descottes [:jdescottes]]

Thanks for the feedback!

What about doing it for custom and revisit the decision against complicated based on DAMP results?

Sounds good!

Note that netmonitor really ought to have its own custom test.

Yes! Honza pointed me to https://bugzilla.mozilla.org/show_bug.cgi?id=1419327. I can try to take care of it as a follow up.

If I recall correctly we aren't waiting for anything corrrectly for this panel, so we most likely only measure the time it takes to load the document and not to update the panel.

Yes, I also noticed that when I was investigating why this test was slightly faster with iframes.

Comment 4

[Julian Descottes [:jdescottes]]

I am actively working on the blocking bugs for this one, so I'll take it.

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1589011 - Use a custom domain name in DAMP tests

We can stop using the exact host:port URL and rely on the proxy for all DAMP calls

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1589011 - Use iframes in DAMP custom.* tests

The inspector custom test is duplicated to have one version with iframes and one version without iframes.

Comment 7

[Chris Peterson [:cpeterson]]

Enabling Talos is a prerequisite for enabling Fission in Nightly (M6).

Comment 8

[Julian Descottes [:jdescottes]]

Discussing with :ochameau we realized that adding (remote) frames in our DAMP tests might block running Talos on Fission platforms.
It's very likely that we'll want to get Talos + fission running before devtools is really ready to debug remote iframes.
Asked about this in Bug 1594631.

If this is correct, it means the tweaks and new tests we do for now should stick to same-site iframes.
We would switch to remote iframes when DevTools are actually able to run those tests both with and without Fission.

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1589011 - Use same-site iframes in DAMP custom.* tests

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f49378f3504
Use a custom domain name in DAMP tests r=ochameau
https://hg.mozilla.org/integration/autoland/rev/d230c5d5eec5
Use same-site iframes in DAMP custom.* tests r=ochameau

Comment 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/2f49378f3504
https://hg.mozilla.org/mozilla-central/rev/d230c5d5eec5