Closed Bug 1154747 Opened 9 years ago Closed 9 years ago

TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/session-store/test-session-store.js | test-session-store.js::test_message_pane_height_persistence

Categories

(Thunderbird :: Testing Infrastructure, defect)

Product:
Thunderbird
Component:
Testing Infrastructure
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird38 fixed, thunderbird39 fixed, thunderbird40 fixed, thunderbird41 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 41.0
Tracking Flags:
Tracking Status
thunderbird38 --- fixed
thunderbird39 --- fixed
thunderbird40 --- fixed
thunderbird41 --- fixed

People

(Reporter: jcranmer, Assigned: mkmelin)

References

Details

(Keywords: intermittent-failure, regression)

Attachments

(1 file)

bug1154747_test_message_pane_height_persistence.patch
1.98 KB, patch
rkent
: review+
rkent
: approval-comm-aurora+
rkent
: approval-comm-beta+
rkent
: approval-comm-esr38+
Details | Diff | Splinter Review 
More unstarred test perma-fails.
Depends on: 1154799
Apparently from bug 1130852.
Blocks: 1130852
Keywords: regression
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED

        Attachment #8599276 -
        Flags: review?(acelists)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8599276 [details] [diff] [review]
bug1154747_test_message_pane_height_persistence.patch

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

I always thought the test's setupModule() is called after all of TB is already up. So how does this patch work? Isn't calendar checking the pref and showing the panel before you have a chance to set it to false in the test? Or is there some timeout so that calendar checks the pref later after startup?

    
    

    
  
Maybe a good way to fix this for good would be to manually hide the whole notification bar element for this test, because it will always get in the way and we'd have to fix it for every new bar that gets added.

    
    

    
  
So the problem occurs at http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/session-store/test-session-store.js#243

I think it compares the value from where the notification hasn't come up yet, but I haven't verified it. 

Setting the pref works because it's not the first 3pane. We keep an addressbook window open not to shut down and then close and open 3panes...

02:26:09 INFO - SUMMARY-UNEXPECTED-FAIL | test-session-store.js | test-session-store.js::test_message_pane_height_persistence
02:26:09 INFO - EXCEPTION: The message pane height should be 226, but is actually 233. The oldHeight was: 352: '226' != '233'.
02:26:09 INFO - at: test-folder-display-helpers.js line 2850
02:26:09 INFO - assert_true test-folder-display-helpers.js:2850 11
02:26:09 INFO - assert_equals test-folder-display-helpers.js:2837 3
02:26:09 INFO - test_message_pane_height_persistence test-session-store.js:243 1

    
    

    
  
(In reply to Philipp Kewisch [:Fallen] from comment #249)
> Maybe a good way to fix this for good would be to manually hide the whole
> notification bar element for this test, because it will always get in the
> way and we'd have to fix it for every new bar that gets added.

The test is opening and closing windows, so it's probably easiest to do what i've proposed. Or find a way to take the notification bar height into consideration when needed.

    
    

    
  
Comment on attachment 8599276 [details] [diff] [review]
bug1154747_test_message_pane_height_persistence.patch

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

I think I can agree with Fallen here. When this specific test knows it is testing the window height, let it make sure it is stable. Let's hide the whole notification bar, but only inside test_message_pane_height_persistence(). The other tests do not care, so let them get the default state of UI.

        Attachment #8599276 -
        Flags: review?(acelists) → review-

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8599276 [details] [diff] [review]
bug1154747_test_message_pane_height_persistence.patch

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

I spent some time debugging this bug can't get it to work properly. E.g. setting the pref ONLY for the failing test doesn't work either, and frankly I don't really understand the details of the test. All in all, IMO it's not time well spent trying to figure out how to best hide the notification bar. So I'd just go with the original approach. If anybody wants to take the other approach, feel free to do so.

        Attachment #8599276 -
        Flags: review- → review?(acelists)

    
    

    
      
  

    
    

    
  
Is the failure currently not happening on trunk?

    
    

    
  
(In reply to :aceman from comment #264)
> Is the failure currently not happening on trunk?

No, but only because lightning is pretty much hosed on trunk so it never puts up the integration dialog...

    
    

    
  
Comment on attachment 8599276 [details] [diff] [review]
bug1154747_test_message_pane_height_persistence.patch

I'd love to get this landed since these failures are occurring on multiple trees. Looked it over, makes sense to me, so if you need a review you have mine.

        Attachment #8599276 -
        Flags: review+

        Attachment #8599276 -
        Flags: approval-comm-beta?

        Attachment #8599276 -
        Flags: approval-comm-aurora?

    
    

    
      
  

    
  
Keywords: checkin-needed

    
  

        Attachment #8599276 -
        Flags: review?(acelists)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
https://hg.mozilla.org/comm-central/rev/8ae02b209d4a -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 9 years ago

          status-thunderbird41:
          --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 41.0

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: