1402846 - Tests do not wait for TextEditor.update to complete

Status: NEW

(DevTools :: Inspector, task, P2)

Description

[Alexandre Poirot [:ochameau]]

While working on bug 1387128 I realized that we were not waiting correctly for TextEditor.update() completion. Which leads to test failures.

http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/devtools/client/inspector/markup/views/text-editor.js#102-107
  this.value.textContent = str;

This is done asynchronously, and nothing in these tests was waitng correctly for that to happen:

http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/devtools/client/inspector/test/head.js#791-801

http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/devtools/client/inspector/markup/test/browser_markup_textcontent_display.js#76

Bug 1387128 slightly changes promise execution order and make these tests fail.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1402846 - Wait for TextEditor updates in inspector codebase.

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

Comment 2

[Alexandre Poirot [:ochameau]]

Last try was almost green:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4a18618bf53ee44028134fb50bb8d576d42abfd&selectedJob=133150363
But there may be some fixes left to do in tests.

Comment 3

[Alexandre Poirot [:ochameau]]

Got a green try with some tweaks to devtools/client/inspector/markup/test/browser_markup_tag_edit_09.js.
But still suspect devtools/client/inspector/test/browser_inspector_addNode_03.js from being intermittent.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e39168c60aa540d75aff388c81317da386d50bbb&selectedJob=133606097

Comment 4

[Alexandre Poirot [:ochameau]]

Getting tests to stay green is challenging, still issues with addNode test:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d49449179e7854691838c412057eda01d304729&selectedJob=133673519

Comment 5

[Patrick Brosset <:pbro>]

Oh my... I had not seen this review request, sorry I missed it. And now I'm going to have a hard time reviewing unfortunately. Sorry, my bad. But today isn't good, and I'll be less available next week (traveling), so it's better if someone else picks this up at this stage.
I will suggest Gabriel because he knows the inspector code.
I'll just ask one question after looking very quickly at the code: is there a risk that this will make node selection more sequential and take more time? await will interrupt the function execution for as long as the promise has not resolved, while before we were doing more things in parallel, right? But maybe our parallel steps were actually not.

Comment 6

[Alexandre Poirot [:ochameau]]

(In reply to Patrick Brosset <:pbro> from comment #5)
> I'll just ask one question after looking very quickly at the code: is there
> a risk that this will make node selection more sequential and take more
> time?

Yes, that's possible in theory.

> await will interrupt the function execution for as long as the promise
> has not resolved, while before we were doing more things in parallel, right?
> But maybe our parallel steps were actually not.

Also likely true. We may parallelize things between parent and content processes
if this code involves doing multiple RDP requests. But at the end both client and actors
run on the main thread, so that they will end up doing only one thing at a time.
My experience on bug 1393086 proved that it is actually extremelly challenging to ensure
that code really run in parallel between client and actors and we are missing low level
API to really be able to do that in a deterministic way.

It will be hard to notice any user visible difference,
but I'll try to see if we can have this covered by DAMP.

On my side, I'm more concerned about the possible races that such patch could introduce
rather than feature completion speed. I'm still having a hard time getting tests to pass.
I'm surprised as waiting for more things to happen should make test writing easier
as you better know when an action is fully completed.

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8912234 [details]
Bug 1402846 - Wait for TextEditor updates in inspector codebase.

https://reviewboard.mozilla.org/r/183608/#review190632

The changes look good to me, but I am gonna pass the final review to bgrins since he also has deeper knowledge regarding the markup compare to me.

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8912234 [details]
Bug 1402846 - Wait for TextEditor updates in inspector codebase.

I'm going to clear this review to reflect reality, because I'm not going to be able to help figure out why the addNode tests are failing in the short term.  I'm generally in favor of the change though.

If nothing else, getting some of the async/await conversion split out into a separate patch and landed could help prevent bitrot and make the existing code easier to follow while the tests get figured out here.