1007057 - (build-am2) build appmgr v2 (keep it preffed-off)

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Paul Rouget [:paul]]

appmgr v2 is not built by default.

This bug will track bugs we want to fix before building it.

Note: enable appmgr v2 at build time should not enable appmgr v2 in the UI.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Filter on 86b7095e-2bd0-499e-a704-d00f2524aeef / PAUL STOP SETTING QA CONTACT TO THE DEVTOOLS COMPONENT'S WATCHERS EMAIL FOR BUGS YOU FILE :)

Comment 3

[Paul Rouget [:paul]]

Attachment: v1

Comment 4

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=3bfffc06abe7

Comment 5

[Paul Rouget [:paul]]

Attachment: v2

Comment 6

[Paul Rouget [:paul]]

Comment on attachment 8431465 [details] [diff] [review]
v2

Alex, I'm removing the call to showRuntimePanel() as it fails sometimes. Basically, the popups tries to open, and during popupshowing, it's cancelled. It fails very very rarely (to reproduce, I need to run the test hundreds of time and clear my disk cache before running the tests, and it only happens on Linux), so it makes it very difficult to figure out what's going on. I thought that it could be a race with the busyUntil function that hides the popups, but if I comment out hidePopup(), it still happens. So it's not related to busyUntil. My guess is it's a focus issue.

Because it's hard to figure out what's going on and because calling showRuntimePanel is not required in this test, and because I don't want to block webidde just because of this, I just removed the call.

Comment 7

[Paul Rouget [:paul]]

Comment on attachment 8431465 [details] [diff] [review]
v2

Alex, there's another timeout error. No need to review until I get a green try.
Michael, can you just review the build part?

Comment 8

[Paul Rouget [:paul]]

Triggered moth mochitest-o: https://tbpl.mozilla.org/?tree=Try&rev=3bfffc06abe7
Try with more logs: https://tbpl.mozilla.org/?tree=Try&rev=7d37004965c3

Comment 9

[Michael Shal [:mshal]]

Comment on attachment 8431465 [details] [diff] [review]
v2

Looks fine :)

Comment 10

[Paul Rouget [:paul]]

Summary of the situation:
* test_runtime.html: play button is enabled 5 - intermittent on linux
* test_import.html: error: Already added

Other timeouts are cause by the "Already added" error.

Comment 11

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=8c45c93a0793

Comment 12

[Paul Rouget [:paul]]

Figured that we were not emptying IDB correctly: https://tbpl.mozilla.org/?tree=Try&rev=b741facb286c

Comment 13

[Paul Rouget [:paul]]

Attachment: v3

Several oth runs: https://tbpl.mozilla.org/?tree=Try&rev=b741facb286c
All mochitests: https://tbpl.mozilla.org/?tree=Try&rev=2e5263124053

All green, but for some reason, osx 1.8 are taking forever to run the test (been waiting for 7 hours).

Summary of the changes I made:
- removeAllProjects (used to clean a test), was returning too early, next test could fail because IDB was not empty
- in test_runtime, I'm now using the packaged app, which doesn't have errors in it (the hosted app is not valid in the test, because of the chrome:// URL) and use the promise of busyUI() to block on testing the buttons

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8434726 [details] [diff] [review]
v3

Review of attachment 8434726 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

Few comments that sounds more related to whenever we will turn the pref on:
 - downloading templates fails by default (I imagine it is still being worked on?),
 - my app currently registered into app manager v1 get into v2 but all being named "--".

Otherwise, I played with WebIDE and everything looks great so far.

Comment 15

[Paul Rouget [:paul]]

(In reply to Alexandre Poirot (:ochameau) from comment #14)
> Comment on attachment 8434726 [details] [diff] [review]
> v3
> 
> Review of attachment 8434726 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great!
> 
> Few comments that sounds more related to whenever we will turn the pref on:
>  - downloading templates fails by default (I imagine it is still being
> worked on?),

Yep. Waiting for the templates.

>  - my app currently registered into app manager v1 get into v2 but all being
> named "--".

I thought I fixed that... I'll file a bug.

Comment 16

[Paul Rouget [:paul]]

We will add the templates later.

Comment 17

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/88a641ee5cbe

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/88a641ee5cbe

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out for frequent test timeouts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ff4cebbcd43

https://tbpl.mozilla.org/php/getParsedLog.php?id=41275741&tree=Mozilla-Inbound etc...

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/2ff4cebbcd43

Comment 21

[Paul Rouget [:paul]]

2 failures for reference:
https://tbpl.mozilla.org/php/getParsedLog.php?id=41275741&full=1&branch=mozilla-inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41245640&tree=Mozilla-Central

Comment 22

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=0e0f5a97a96e

Just added some logs to get more info about what's going on.

Comment 23

[Paul Rouget [:paul]]

Well, I can't reproduce the orange. Now trying with all the mochitests: https://tbpl.mozilla.org/?tree=Try&rev=23f5c31f1025

Comment 24

[Alexandre Poirot [:ochameau]]

On today's windows nightly, if I turn webide pref on, I can't open the app manager anymor:

  No chrome package registered for chrome://webide/content/webide.xul

Comment 25

[Paul Rouget [:paul]]

(In reply to Alexandre Poirot (:ochameau) from comment #24)
> On today's windows nightly, if I turn webide pref on, I can't open the app
> manager anymor:
> 
>   No chrome package registered for chrome://webide/content/webide.xul

It's been backed out.

Comment 26

[Paul Rouget [:paul]]

Attachment: add some logs to the test

Comment 27

[Paul Rouget [:paul]]

Ryan, I didn't manage to reproduce the orange (ran the test ~100 times, see try builds). I'd like to re-land this patch with some extra logs if that works for you.

Comment 28

[Patrick Brosset <:pbro>]

Comment on attachment 8438250 [details] [diff] [review]
add some logs to the test

Review of attachment 8438250 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/webide/content/webide.js
@@ +172,1 @@
>      }, 30000);

Shouldn't this be moved as a const at the top of file somewhere?

::: browser/devtools/webide/test/test_import.html
@@ +62,5 @@
>  
>              SimpleTest.finish();
> +        }).then(null, e => {
> +          ok(false, "Exception: " + e);
> +          SimpleTest.finish();

Good catch!

Comment 29

[Patrick Brosset <:pbro>]

Comment on attachment 8438250 [details] [diff] [review]
add some logs to the test

Review of attachment 8438250 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/webide/test/head.js
@@ +34,5 @@
>  
>    win.addEventListener("load", function onLoad() {
>      win.removeEventListener("load", onLoad);
>      info("WebIDE open");
> +    SimpleTest.requestCompleteLog();

One question about this though: why is this required in this test?

Comment 30

[Paul Rouget [:paul]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #29)
> Comment on attachment 8438250 [details] [diff] [review]
> add some logs to the test
> 
> Review of attachment 8438250 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/webide/test/head.js
> @@ +34,5 @@
> >  
> >    win.addEventListener("load", function onLoad() {
> >      win.removeEventListener("load", onLoad);
> >      info("WebIDE open");
> > +    SimpleTest.requestCompleteLog();
> 
> One question about this though: why is this required in this test?

Because in the failure, it says:

> 04:38:29     INFO -  119 INFO TEST-INFO | if you need more context, please use SimpleTest.requestCompleteLog() in your test

I'm not sure how useful it is though.

Comment 31

[Paul Rouget [:paul]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #28)
> Comment on attachment 8438250 [details] [diff] [review]
> add some logs to the test
> 
> Review of attachment 8438250 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/webide/content/webide.js
> @@ +172,1 @@
> >      }, 30000);

I'll address that in bug 1021528.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #27)
> Ryan, I didn't manage to reproduce the orange (ran the test ~100 times, see
> try builds). I'd like to re-land this patch with some extra logs if that
> works for you.

OK

Comment 33

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/3bd2b5b8e46d
https://hg.mozilla.org/integration/fx-team/rev/3c709608bb8a

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3c709608bb8a
https://hg.mozilla.org/mozilla-central/rev/3bd2b5b8e46d