817562 - Run all tests with the toolbox undocked

Status: RESOLVED WORKSFORME

(DevTools :: General, defect, P2)

Description

[Paul Rouget [:paul]]

We are running the devtools tests with the toolbox docked. It would be great to re-run the test with the toolbox undocked as well.

If we re-run all the tests, that will multiply by two the time spend testing the tools. Maybe we could flag some tests as "test-once" (maybe the one less likely to be affected by the undock mechanism).

Comment 1

[Paul Rouget [:paul]]

In Makefile.in, we could define the tools to run twice, the one to run once, run all the tests, and then do some magic to change the pref for the second run.

Comment 2

[Girish Sharma [:Optimizer]]

An idea: Have an extra test at the end of the list of tests in the makefile. That test toggles the specific preference that will be used in the tests to have the toolbox docked or undocked. Since that test will also run 2 times, the preference will be restored to default state after all tests have completed.

Comment 3

[Joe Walker [:jwalker]]

I'm currently removing all the hardcoded hosts from the unit tests so we should be able to flip where the toolbox is located fairly easily.

Aside from specific tests of toolbox hosting, we don't currently know how much we need to run normal tests in different locations.

Once we can simply run the tests with different hostings, we should be in a position to know how important it is to do this routinely.

Comment 4

[Joe Walker [:jwalker]]

Attachment: v1

This patch isn't a candidate for central, but should inform us of how likely tests are to break in non-default hostings.

Comment 5

[Joe Walker [:jwalker]]

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

Comment 6

[Paul Rouget [:paul]]

I looked at the raw logs. I only see debugger failures. This is surprising. Am I missing something?

Comment 7

[Victor Porof [:vporof][:vp]]

(In reply to Paul Rouget [:paul] from comment #6)
> I looked at the raw logs. I only see debugger failures. This is surprising.
> Am I missing something?

Maybe the toolbox window needs to be focused?

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

2 of the Mac failures are probably caused by bug 726616, judging by the screenshots. For the other platforms, I think we can blame the leaktest failure to the delayed GCLI test cleanups, but we need to investigate the watch expressions that come up next.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

I have fixes for some of these and working on the others. Taking.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Test fixes

This was a largely mechanical change to make sure debugger tests do not rely on events propagating to or from the tab window.

Try run with Joe's patch to launch the toolbox in a separate window:
https://tbpl.mozilla.org/?tree=Try&rev=c226a50ba9ce

I should note that tests pass locally with both a detached and an attached toolbox.

Comment 11

[Victor Porof [:vporof][:vp]]

(In reply to Panos Astithas [:past] from comment #10)

There's a bit of carnage in that try run. Not sure if it's because Joe's patch, or this one, but the changes look good in principle.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

I took a look at the oranges and it looks like some things regressed in the last two weeks after Joe's try push. There is only one failing debugger test in Windows and this is while the test has finished successfully, so it looks like the test just takes longer in those cases with a detached toolbox.

I can reproduce the first failures locally even when I only run the framework tests, without interference from previously run debugger tests. It should also be noted that this patch only touches tests, not product code, so I feel confident that the new failures are not caused by this patch.

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Verified that without my patch, the other failures still occur. Landed the debugger test fixes:
https://hg.mozilla.org/integration/fx-team/rev/a030d7a75344

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/a030d7a75344

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Debugger tests are OK now, so I'm giving this back to Joe.

Comment 16

[Joe Walker [:jwalker]]

Comment on attachment 698416 [details] [diff] [review]
Test fixes

This is a very old patch. Do we know it applies etc?
I took checkin+ off in case this was a mistake. We can easily add it back if it wasn't.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 698416 [details] [diff] [review]
Test fixes

checkin+ indicates that the patch has landed (see comment 13), not that I'm asking someone to land it (that's checkin?). What I should have explained is that I added this so that bugmotodo.org stops showing this in my "To Check In" list :-)

Comment 18

[Joe Walker [:jwalker]]

(In reply to Panos Astithas [:past] from comment #17)
> Comment on attachment 698416 [details] [diff] [review]
> Test fixes
> 
> checkin+ indicates that the patch has landed (see comment 13), not that I'm
> asking someone to land it (that's checkin?). What I should have explained is
> that I added this so that bugmotodo.org stops showing this in my "To Check
> In" list :-)

I see. I didn't want it to be seen as "permission to land", this is such an old bug that I was surprised by the traffic. Thanks.

Comment 19

[Joe Walker [:jwalker]]

I won't have time to work on this.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

I ran all of our tests again with attachment 693344 [details] [diff] [review] applied and everything works. I'm closing the bug, but feel free to reopen if you disagree.