Closed Bug 1399460 Opened 2 years ago Closed 2 years ago

devtools reps: update bundle to v0.13.0

Categories

(DevTools :: Shared Components, defect)

defect
Not set

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

(Blocks 3 open bugs)

Details

Attachments

(4 files)

No description provided.
Blocks: 1388831, 1307929, 1391077
No longer blocks: 1307931
No longer depends on: 1386525
Comment on attachment 8907974 [details]
Bug 1399460 - Adapt the console to the new Reps bundle. .

https://reviewboard.mozilla.org/r/179648/#review185044

LGTM

Honza
Attachment #8907974 - Flags: review?(odvarko) → review+
Comment on attachment 8908215 [details]
Bug 1399460 - Fix browser_ext_devtools_inspectedWindow_eval_bindings.js failure.

https://reviewboard.mozilla.org/r/179882/#review185100

This looks almost ready to me, follows a couple of comments related to some additional assertions and logged messages that we can add to the test, so that this test can produce more detailed information when it fails because of some unexpected behavior of the rendered object tree.

::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_eval_bindings.js:147
(Diff revision 1)
> +      // We need to wait for the object to be expanded so we don't call the server on a closed connection.
> +      const [oi] = objectInspectors;
> +      let nodes = oi.querySelectorAll(".node");
> +
> +      // The tree can still be collapsed since the properties are fetched asynchronously.
> +      if (nodes.length === 1) {

What if nodes.length is 0 or > 1?

Even if it is something that should never happen, I'm wondering if a regression could lead to a test that is green when it should not or a test failure (or an intermittent failure) for which the causes are not immediately clear.

How about adding the following behaviors to this test:

- fail explicitly when the initial nodes.length is not the expected one (with an additional assertion on nodes.length so that we get a clear failure message when something unexpected happens here)

- emit a log (e.g. something like `info("Waiting for the object preview render to be completed");`) before the `await` on the MutationObserver promise

- test the value of `oi.querySelectorAll(".node).length` after the  MutationObserver promise has been resolved and fail the test explicitly if its value is not the expected one.

::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_eval_bindings.js:151
(Diff revision 1)
> +      // The tree can still be collapsed since the properties are fetched asynchronously.
> +      if (nodes.length === 1) {
> +        // If this is the case, we wait for the properties to be fetched and displayed.
> +        await new Promise(resolve => {
> +          const observer = new MutationObserver(mutations => {
> +            resolve(mutations);

The resolved `mutations` doesn't seem to be used anywhere.

If it is not needed we can just omit it, but I'm also wondering if we should look inside the mutations data and only resolve the promise when we have received the mutation data that we expect.
Attachment #8908215 - Flags: review?(lgreco)
Comment on attachment 8908215 [details]
Bug 1399460 - Fix browser_ext_devtools_inspectedWindow_eval_bindings.js failure.

https://reviewboard.mozilla.org/r/179882/#review185136

Thanks Nicolas!

r+ with green try run for the last version of the changes applied to this test
Attachment #8908215 - Flags: review?(lgreco) → review+
Comment on attachment 8908216 [details]
Bug 1399460 - Fix for browser_webconsole_check_stubs_console_api.

https://reviewboard.mozilla.org/r/179884/#review185400

R+ but of course, setTimeout in a test always feels bad.

Honza
Attachment #8908216 - Flags: review?(odvarko) → review+
Attachment #8907973 - Flags: review?(nchevobbe)
Comment on attachment 8907973 [details]
Bug 1399460 - Release: Update reps bundle to 0.13.0.

https://reviewboard.mozilla.org/r/179646/#review185406

Everything was already reviewed in Github
Attachment #8907973 - Flags: review?(nchevobbe) → review+
Comment on attachment 8908216 [details]
Bug 1399460 - Fix for browser_webconsole_check_stubs_console_api.

(In reply to Jan Honza Odvarko [:Honza] from comment #14)
> Comment on attachment 8908216 [details]
> Bug 1399460 - Fix for browser_webconsole_check_stubs_console_api.
> 
> https://reviewboard.mozilla.org/r/179884/#review185400
> 
> R+ but of course, setTimeout in a test always feels bad.
> 
> Honza

I agree, this was a bad evening attempt to get rid of a test failure.
I changed the test which now hide all logs messages so there won't be unwanted client/server communication.
Does it look reasonable to you Honza ?
Attachment #8908216 - Flags: review+ → review?(odvarko)
Comment on attachment 8908216 [details]
Bug 1399460 - Fix for browser_webconsole_check_stubs_console_api.

https://reviewboard.mozilla.org/r/179884/#review185410

Yes, this is better, thanks!

Honza
Attachment #8908216 - Flags: review?(odvarko) → review+
Assignee: nobody → nchevobbe
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e843a11f6fde
Release: Update reps bundle to 0.13.0. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/0f7830472380
Adapt the console to the new Reps bundle. r=Honza.
https://hg.mozilla.org/integration/autoland/rev/e7d1f0e04aae
Fix browser_ext_devtools_inspectedWindow_eval_bindings.js failure. r=rpl
https://hg.mozilla.org/integration/autoland/rev/8b0058e2a4a8
Fix for browser_webconsole_check_stubs_console_api. r=Honza
Blocks: 1391649
Depends on: 1450541
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.