1209791 - TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[aleth [:aleth]]

EXCEPTION: Timeout waiting for modal dialog to open
https://dxr.mozilla.org/comm-central/source/mail/test/mozmill/content-tabs/test-content-tab.js#162

Looks like this is caused by bug 636905.

Comment 1

[:aceman]

Thanks for investigating this. So you suspect the onbeforeunload event is not run, therefore we do not get a dialog (probably containing the event.returnValue text). We could probably interact with the tab to fix the test. But do we need to test this that event? I could not find any use of it in the base code. There is only a comment at http://mxr.mozilla.org/comm-central/source/mail/base/content/tabmail.xml#697.

Comment 2

[aleth [:aleth]]

Ask a peer of the code whether the test can be disabled? Otherwise you'd probably fix it like so: https://hg.mozilla.org/mozilla-central/rev/ef432f51a591

Comment 3

[aleth [:aleth]]

Attachment: TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload

Comment 4

[:aceman]

Comment on attachment 8669191 [details] [diff] [review]
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload

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

As our code does mention the possibility of running the onbeforeunload event, I am not sure if we have some code that needs to run always, not only if we interacted with the tab. We may need to make this pref global for the app (in defaults.js) unless we audit all potential users of it.
So f+ that this fixes the particular test, but it still needs to be decided if this is the right way to do it.

Comment 5

[Richard Marti (:Paenglab)]

Comment on attachment 8669191 [details] [diff] [review]
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload

I'm far away to be able to review code in tests, so removing me. I've never wrote a test and can't say if something is correct or wrong.

Magnus surely is the correct person to review this.

Comment 6

[:aceman]

The question was not about the test but about tabs needing onbeforeunload event always.

Comment 7

[Richard Marti (:Paenglab)]

Also for this question, I don't know enough to say what this change would imply.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 8669191 [details] [diff] [review]
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload

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

Looks reasonable I think. Please also clearUserPref after the test is done.

Comment 9

[aleth [:aleth]]

(In reply to Magnus Melin from comment #8)
> Looks reasonable I think. Please also clearUserPref after the test is done.

That's not needed afaik, each test runs in a fresh environment.

Comment 10

[:aceman]

No, in mozmill all tests in a directory run in the same environment. Each test directory starts fresh.

Comment 11

[aleth [:aleth]]

(In reply to :aceman from comment #10)
> No, in mozmill all tests in a directory run in the same environment. Each
> test directory starts fresh.

Thanks! I guess it's only for xpcshell then.

Comment 12

[aleth [:aleth]]

Attachment: TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload

Comment 13

[aleth [:aleth]]

Attachment: TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

What is the status of this bug? thunderbird-tree-status says it is checkin-needed, but it doesn't seem so.

Comment 15

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/071eec01aca961c0d603b94c41a89a17645f4af6
Bug 1209791 - TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload. r=mkmelin a=aleth

Comment 16

[aleth [:aleth]]

(In reply to Philipp Kewisch [:Fallen] from comment #14)
> What is the status of this bug? thunderbird-tree-status says it is
> checkin-needed, but it doesn't seem so.

The tree was closed...

Comment 17

[Stefan Sitter [:ssitter]]

Failing on aurora too
> https://treeherder.mozilla.org/logviewer.html#?job_id=21364&repo=comm-aurora

Comment 18

[Kent James (:rkent)]

Comment on attachment 8672255 [details] [diff] [review]
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload

https://hg.mozilla.org/releases/comm-aurora/rev/5e0f8ffa1acb