1399460 - devtools reps: update bundle to v0.13.0

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect)

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1399460 - Release: Update reps bundle to 0.13.0.

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

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1399460 - Adapt the console to the new Reps bundle. .

This is mainly changing the props we pass to the ObjectInspector,
and removing actions, state properties and reducers that we used to
handle in the console but are now managed by the ObjectInspector
itself.

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

Comment 3

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

Comment on attachment 8907974 [details]
Bug 1399460 - Adapt the console to the new Reps bundle. .

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

LGTM

Honza

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8907973 [details]
Bug 1399460 - Release: Update reps bundle to 0.13.0.

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

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8907974 [details]
Bug 1399460 - Adapt the console to the new Reps bundle. .

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

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1399460 - Fix browser_ext_devtools_inspectedWindow_eval_bindings.js failure.

The bug was failing because we were calling the server but the connection was already closed.
In order to prevent such things, we check the number of nodes of the tree, and if it has only
one, then we wait on a mutation that will ensure that server calls are done.

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

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1399460 - Fix for browser_webconsole_check_stubs_console_api.

This patch makes the test one second at the end before closing the toolbox so we don't
have errors due to the connection to the server being closed.

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

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

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.

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8908215 [details]
Bug 1399460 - Fix browser_ext_devtools_inspectedWindow_eval_bindings.js failure.

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

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8908216 [details]
Bug 1399460 - Fix for browser_webconsole_check_stubs_console_api.

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

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

TRY seems fine now https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba249ef7ac495363c69083f4525e057ba8641821

Comment 12

[Luca Greco [:rpl] [:luca] [: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

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

TRY is over and fine https://treeherder.mozilla.org/#/jobs?repo=try&revision=b115bdabb88022333a4033d33cebb2aade6298ef&selectedJob=131220220

Comment 14

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

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

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8907973 [details]
Bug 1399460 - Release: Update reps bundle to 0.13.0.

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

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8907974 [details]
Bug 1399460 - Adapt the console to the new Reps bundle. .

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

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8908215 [details]
Bug 1399460 - Fix browser_ext_devtools_inspectedWindow_eval_bindings.js failure.

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

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8908216 [details]
Bug 1399460 - Fix for browser_webconsole_check_stubs_console_api.

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

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8907973 [details]
Bug 1399460 - Release: Update reps bundle to 0.13.0.

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

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8907974 [details]
Bug 1399460 - Adapt the console to the new Reps bundle. .

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

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8908215 [details]
Bug 1399460 - Fix browser_ext_devtools_inspectedWindow_eval_bindings.js failure.

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

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8908216 [details]
Bug 1399460 - Fix for browser_webconsole_check_stubs_console_api.

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

Comment 23

[Nicolas Chevobbe [: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

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

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 ?

Comment 25

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

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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e843a11f6fde
https://hg.mozilla.org/mozilla-central/rev/0f7830472380
https://hg.mozilla.org/mozilla-central/rev/e7d1f0e04aae
https://hg.mozilla.org/mozilla-central/rev/8b0058e2a4a8