Open Bug 904152 Opened 11 years ago Updated 2 years ago

TEST-UNEXPECTED-FAIL | test-message-filters.js | test-message-filters.js::test_customize_toolbar_doesnt_double_get_mail_menu

Categories

(Thunderbird :: Testing Infrastructure, defect)

Product:
Thunderbird
Component:
Testing Infrastructure
Platform:
x86
macOS
Type:
defect

Tracking

(Not tracked)

Status:
ASSIGNED

People

(Reporter: standard8, Assigned: aceman)

References

Details

(Keywords: intermittent-failure, Whiteboard: [leave open])

Attachments

(2 files, 4 obsolete files)

[checked in] Fix
2.31 KB, patch
standard8
: review+
Details | Diff | Splinter Review
patch v4
9.01 KB, patch
Details | Diff | Splinter Review 
We've been seeing this occasionally, only on Linux I believe:

https://tbpl.mozilla.org/php/getParsedLog.php?id=26441482&tree=Thunderbird-Beta#error0

SUMMARY-UNEXPECTED-FAIL | test-message-filters.js | test-message-filters.js::test_customize_toolbar_doesnt_double_get_mail_menu
  EXCEPTION: Timeout waiting for popup to open
    at: utils.js line 447
       TimeoutError utils.js 447
       waitFor utils.js 485
       wait_for_popup_to_open test-folder-display-helpers.js 1364
       check_getAllNewMsgMenu test-message-filters.js 115
       test_customize_toolbar_doesnt_double_get_mail_menu test-message-filters.js 136
       Runner.prototype.wrapper frame.js 582
       Runner.prototype._runTestModule frame.js 652
       Runner.prototype.runTestModule frame.js 698
       Runner.prototype.runTestDirectory frame.js 522
       runTestDirectory frame.js 704
       Bridge.prototype._execFunction server.js 179
       Bridge.prototype.execFunction server.js 183
Can we use this bug to continue with the issues from 871266 as it is in the same test? The patch tries to fix OS X but also makes other platforms more robust so could help here too.
Flags: needinfo?(mbanner)
Sure, no objection to that. Just didn't want to confuse the other bug by more fixes.
Flags: needinfo?(mbanner)
Attached patch patch v1 (obsolete) — — Splinter Review 
So this is the "fix for OS X v8" from bug 871266. I am still not convinced it causes the test failure from bug 871266 comment 146 as I instrumented click_menus_in_sequence and it is not called from test-message-header.js . Or there is some strange interaction going on. For linux, this changes the handling of the "get new messages menu" from click(popup) to click_menus_in_sequence(popup array) which is now fortified with the explicit focus setting from bug 889752.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #789082 - Flags: review?(mbanner)
I pushed this to try and got the same failure:

https://tbpl.mozilla.org/php/getParsedLog.php?id=27106801&tree=Thunderbird-Try#error0

TEST-START | /builds/slave/talos-slave/test/build/mozmill/message-header/test-message-header.js | test_add_tag_with_really_long_label
Step Pass: {"function": "Controller.keypress()"}
Test Failure: Header columns didn't change width!  80 == 80
TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/mozmill/message-header/test-message-header.js | test-message-header.js::test_add_tag_with_really_long_label

Don't forget we have seen the case that the test files can be run in a different order depending on what platform you're on, so you might want to play around with that a bit.
Attachment #789082 - Flags: review?(mbanner) → feedback-
aceman, what is the current status on this issue? I can tackle this issue if you don't have enough time.
Thanks, you can look at it. I do not have OS X so can't really move forward yet. I still think my patch is the correct think to do (adds some more correctness), but seems to cause some problem. If you can find the needed change to be done on top of my patch then I would be grateful.
Attached patch Fix (obsolete) — — Splinter Review 
Try server results:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=fb82f568695a
Attachment #789082 - Attachment is obsolete: true
Attachment #813001 - Flags: review?(mbanner)
This patch seems to reduce failure rate but does not fix completely.
Attached patch [checked in] Fix — — Splinter Review 
The failure reason is that blur event caused by filter window (or toolbar customization window) is fired soon after opening popup menu. So the popup menu is closed by the event. The following is an example of failure cases.

1. filter window is opened in test_message_filter_shows_newsgroup_server.
2. filter window is closed.
3. test_customize_toolbar_doesnt_double_get_mail_menu is started.
4. popup menu is opened.
5. blur event of the filter window is fired.
6. popup menu is closed by the blur event.
7. FAIL the test!


This patch adds some wait_for_window_focused to avoid this failure and also adds close_popup for the safety.

try server results:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=5f47e990dcac
Attachment #813001 - Attachment is obsolete: true
Attachment #813001 - Flags: review?(mbanner)
Attachment #815723 - Flags: review?(mbanner)
Comment on attachment 815723 [details] [diff] [review]
[checked in] Fix

Shouldn't then the wait_for_window_focused() be part of wait_for_new_window() ?
Or wait for the blur() inside close_window? Why does the blur happen only at step 5. in your steps and not after step 2.?
(In reply to :aceman from comment #68)
> Comment on attachment 815723 [details] [diff] [review]
> Fix
> 
> Shouldn't then the wait_for_window_focused() be part of
> wait_for_new_window() ?

IMHO, wait_for_window_focused should be invoked only if test needs input text (or popup something to avoid blur like this bug).
 
> Or wait for the blur() inside close_window? Why does the blur happen only at
> step 5. in your steps and not after step 2.?

I guess the reason is that spinning main loop is not sufficient at the step 2.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #69)
> (In reply to :aceman from comment #68)
> > Comment on attachment 815723 [details] [diff] [review]
> > Fix
> > 
> > Shouldn't then the wait_for_window_focused() be part of
> > wait_for_new_window() ?
> 
> IMHO, wait_for_window_focused should be invoked only if test needs input
> text (or popup something to avoid blur like this bug).

I mean the test needs focus.
How does the developer know if he needs focus? And even if the test is once written, the focus conditions may change in the future. I think we should make the helper functions robust enough even if they did more work than always necessary. But better be slower and more correct than fail randomly as today.
(In reply to :aceman from comment #71)
> How does the developer know if he needs focus? And even if the test is once
> written, the focus conditions may change in the future. 

I suppose that the test which uses popup soon after window is closed needs focus on linux.

> I think we should
> make the helper functions robust enough even if they did more work than
> always necessary. But better be slower and more correct than fail randomly
> as today.

I suppose we should fix focus handlings on linux not to fire blur event if window has been closed.
Depends on: 179033
Comment on attachment 815723 [details] [diff] [review]
[checked in] Fix

Looks good. I'm happy with this moving us forward. We might need to consider the linux changes at some stage.
Attachment #815723 - Flags: review?(mbanner) → review+
Hiro, is your patch ready for check in?
Flags: needinfo?(hiikezoe)
Whiteboard: [please leave open after checkin]
Yes.
Flags: needinfo?(hiikezoe)
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/eb84c090ffb8
Keywords: checkin-needed
Whiteboard: [please leave open after checkin] → [leave open]
Attached patch patch v2 (obsolete) — — Splinter Review 
OK, I stripped down my patch as it no longer needs the changes to click_menus_in_sequence because that was covered by bug 179033.
Aryx, could you make me a try run of this, all platforms, mozmill tests only?
Attachment #823561 - Flags: feedback?(archaeopteryx)
Attachment #815723 - Attachment description: Fix → [checked in] Fix
Attached patch patch v3 (obsolete) — — Splinter Review 
Aryx, please try this version.
Attachment #823561 - Attachment is obsolete: true
Attachment #823561 - Flags: feedback?(archaeopteryx)
Attachment #823635 - Flags: feedback?(archaeopteryx)
Comment on attachment 823635 [details] [diff] [review]
patch v3

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

OK, so there seems to be quite a mess on the try server. However, the test in question folder-widget/test-message-filters.js does not seem affected by this patch (it still passes on all platforms).

standard8, do you want to do any more tests with this?
Attachment #823635 - Flags: review?(mbanner)
Comment on attachment 823635 [details] [diff] [review]
patch v3

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

Using http://clicky.visophyte.org/tools/arbpl-mozmill-standalone/?log=http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/archaeopteryx@coole-files.de-12f3e8fc95e6/try-comm-central-linux-debug/try-comm-central_ubuntu32_vm-debug_test-mozmill-bm51-tests1-linux-build12.txt.gz at least one of the errors is:

name is not defined

file:///builds/slave/test/build/mozmill/shared-modules/test-window-helpers.js line 1019 _click_menus

Given that, and the fact that the rest of the failures aren't showing on trunk but are also cross-platform, it makes me think that there's still a few issues in this patch.
Attachment #823635 - Flags: review?(mbanner) → review-
aceman, you seem to have forgotten enable test_customize_toolbar_doesnt_double_get_mail_menu on Mac.
Attached patch patch v4 — — Splinter Review 
Yes, thanks. But the patch was broken anyway so this is a new version. I'll push to try server when the tree is buildable again.
Attachment #823635 - Attachment is obsolete: true
Aryx, may I ask for another try run with patch v4?
Flags: needinfo?(archaeopteryx)
OK, at least it did not regress Linux or Windows. But still didn't help on OS X...
Blocks: 929608
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: