Nicolas Chevobbe [:nchevobbe] Assignee
	Description • 7 years ago

      No description provided.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 3 • 7 years ago
mozreview-review

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

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

LGTM

Honza

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 8 • 7 years ago
mozreview-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.

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 11 • 7 years ago

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

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 12 • 7 years ago
mozreview-review

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

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 13 • 7 years ago

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

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 14 • 7 years ago
mozreview-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

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 23 • 7 years ago
mozreview-review

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

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 24 • 7 years ago

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 ?

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 25 • 7 years ago
mozreview-review

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

	Pulsebot
	Comment 26 • 7 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 7 years ago
bugherder

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