Closed Bug 1007057 (build-am2) Opened 5 years ago Closed 5 years ago

build appmgr v2 (keep it preffed-off)

Categories

(DevTools :: WebIDE, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: paul, Assigned: paul)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 999417
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Blocks: 999420
Summary: Build appmgr v2 → build appmgr v2 (keep it preffed-off)
Depends on: 980085
Depends on: 1009486
Filter on 86b7095e-2bd0-499e-a704-d00f2524aeef / PAUL STOP SETTING QA CONTACT TO THE DEVTOOLS COMPONENT'S WATCHERS EMAIL FOR BUGS YOU FILE :)
QA Contact: developer.tools
Depends on: 1010387
Attached patch v1 (obsolete) — Splinter Review
Depends on: 1011464
Depends on: 1011530
Depends on: 1010271
Depends on: 1010174
No longer depends on: 1011464
Depends on: 1012869
Depends on: 1013851
No longer depends on: 1012869
No longer depends on: 1013851
No longer depends on: 1013998
Depends on: 1007218
Depends on: 1012760
Alias: build-am2
Depends on: 1016924
Depends on: 1016903
Depends on: 1017029
No longer depends on: 1016903
No longer depends on: 1012760
Attached patch v2 (obsolete) — Splinter Review
Assignee: nobody → paul
Attachment #8422981 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #8431465 - Flags: review?(mshal)
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.
Attachment #8431465 - Flags: review?(poirot.alex)
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?
Attachment #8431465 - Flags: review?(poirot.alex)
Comment on attachment 8431465 [details] [diff] [review]
v2

Looks fine :)
Attachment #8431465 - Flags: review?(mshal) → review+
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.
Figured that we were not emptying IDB correctly: https://tbpl.mozilla.org/?tree=Try&rev=b741facb286c
Attached patch v3Splinter Review
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
Attachment #8431465 - Attachment is obsolete: true
Attachment #8434726 - Flags: review?(poirot.alex)
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.
Attachment #8434726 - Flags: review?(poirot.alex) → review+
(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.
Depends on: 1020972
No longer depends on: 980085
We will add the templates later.
https://hg.mozilla.org/mozilla-central/rev/88a641ee5cbe
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
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...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 32 → ---
https://tbpl.mozilla.org/?tree=Try&rev=0e0f5a97a96e

Just added some logs to get more info about what's going on.
Well, I can't reproduce the orange. Now trying with all the mochitests: https://tbpl.mozilla.org/?tree=Try&rev=23f5c31f1025
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
(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.
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.
Flags: needinfo?(ryanvm)
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!
Attachment #8438250 - Flags: review+
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?
(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.
(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.
(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
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/mozilla-central/rev/3c709608bb8a
https://hg.mozilla.org/mozilla-central/rev/3bd2b5b8e46d
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.