Intermittent TEST-UNEXPECTED-FAIL | apps/sms/test/marionette/inbox_test.js | Inbox View tests Long list of conversations User could navigate without waiting for the app to be fully loaded AssertionError: expected false to be true

RESOLVED FIXED

Status

Firefox OS
Gaia::SMS
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mikehenrty, Assigned: azasypkin)

Tracking

({intermittent-failure})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MJS])

Attachments

(1 attachment)

Note, when I disable the retry logic in bug 1222215, I see this failing almost every time. And indeed if I run the test locally, I see this same error. I'm not sure why it passes on current master, but we will need to fix this to land bug 1222215.

TEST-UNEXPECTED-FAIL | apps/sms/test/marionette/inbox_test.js | Inbox View tests Long list of conversations User could navigate without waiting for the app to be fully loaded
AssertionError: expected false to be true
    at Function.assert.isTrue (node_modules/chai/lib/chai/interface/assert.js:317:31)
    at Context.<anonymous> (apps/sms/test/marionette/inbox_test.js:72:14)
    at Test.MarionetteTest.run (node_modules/marionette-js-runner/lib/ui.js:25:31)
(Assignee)

Updated

2 years ago
Flags: needinfo?(azasypkin)
(Assignee)

Comment 1

2 years ago
Looks like just a race and this test is kind of sensitive to timing issues - will try to increase number of threads rendered.
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
Flags: needinfo?(azasypkin)
Thanks for picking this up Oleg. I meant to comment that I disabled this test last week: https://github.com/mozilla-b2g/gaia/commit/d4be6321eabdb28d824a0d28e5efef828c1afece
In the mean time, Oleg, maybe it's possible to just skip the failing test and reenable the full file ? (unless all tests are affected)
(Assignee)

Comment 4

2 years ago
(In reply to Julien Wajsberg [:julienw] (PTO -> 2016) from comment #3)
> In the mean time, Oleg, maybe it's possible to just skip the failing test
> and reenable the full file ? (unless all tests are affected)

IIRC there is only one, but I have a WIP [1] that should make it more stable (depends on the PR in bug 1179604). So if it turns out that WIP requires more time to land than I expect - then yeah I think we can just skip this single test and let split view mode tests to run.

[1] https://github.com/azasypkin/gaia/commit/7aa2b76b0fbbbda38b4809c69ecb652080f30e26

Comment 5

2 years ago
Created attachment 8703115 [details] [review]
[gaia] azasypkin:intermittent-gij-tester > mozilla-b2g:master
(Assignee)

Comment 6

2 years ago
Comment on attachment 8703115 [details] [review]
[gaia] azasypkin:intermittent-gij-tester > mozilla-b2g:master

Hey Steve,

How does it look?

Here is Treeherder results for ~30 runs of Gij that includes only all Messages tests - inbox tests are always green, only one attachments test is failing sometimes (bug 1236418): https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=037f91a5acd8d1e67bda5eff12fe58c907de9068

Thanks!
Attachment #8703115 - Flags: review?(schung)
Comment on attachment 8703115 [details] [review]
[gaia] azasypkin:intermittent-gij-tester > mozilla-b2g:master

I'm just wondering about the necessity of the 2 different launch case, but it seems we might still need this if you want to prove that thread still kkp loading after you enter the new message view. How about we check with app ready flag before/after entering another view instead of checking fixed conversation number in the inbox view? Or you prefer to check the actual number in the list since it's more straightforward?
(Assignee)

Comment 8

2 years ago
(In reply to Steve Chung [:steveck] from comment #7)
> Comment on attachment 8703115 [details] [review]
> [gaia] azasypkin:intermittent-gij-tester > mozilla-b2g:master
> 
> I'm just wondering about the necessity of the 2 different launch case, but
> it seems we might still need this if you want to prove that thread still kkp
> loading after you enter the new message view.

I've replied at GitHub, but leave it here is as well:

Our app is marked as ready on "fully-loaded" and in one of the inbox tests we want to check that user can enter Composer or Conversation _before_ all conversation are loaded. We had a bug (bug 1217859) when we waited for the "load" event before allowing user to navigate further, but because of a big number of contact images user had to wait for fully loaded. Does it make sense?

> How about we check with app
> ready flag before/after entering another view instead of checking fixed
> conversation number in the inbox view? Or you prefer to check the actual
> number in the list since it's more straightforward?

Well I have mixed feelings, "app readiness" is a bit too technical to me and implies a variety of things, more than just fully loaded inbox, but don't have really strong opinion on that :) Tell me if you prefer something like "messagesApp.isFullyLoaded()" (under the hood it will use client scope with reduced search timeout to make sure it won't wait for the fully loaded flag to return :)) and I'll make it happen!
I agree that "app readiness" might be too technical here, and we are not necessary to wait for app loaded completely. But having a "messagesApp.isFullyLoaded" method but won't wait for the fully loaded flag might be confusing. Could we just assert that thread number is still increasing after entering another view(query the number before/after new message view and compare)?
(In reply to Steve Chung [:steveck] from comment #9)
> I agree that "app readiness" might be too technical here, and we are not
> necessary to wait for app loaded completely. But having a
> "messagesApp.isFullyLoaded" method but won't wait for the fully loaded flag
> might be confusing. Could we just assert that thread number is still
> increasing after entering another view(query the number before/after new
> message view and compare)?

Replied at GitHub with more details :)
Comment on attachment 8703115 [details] [review]
[gaia] azasypkin:intermittent-gij-tester > mozilla-b2g:master

Considering that we might not have apparently better way for verification now, current assertion is good enough for me. Let's land this fix now and focus on other intermittent issues.
Attachment #8703115 - Flags: review?(schung) → review+
(In reply to Steve Chung [:steveck] from comment #11)
> Comment on attachment 8703115 [details] [review]
> [gaia] azasypkin:intermittent-gij-tester > mozilla-b2g:master
> 
> Considering that we might not have apparently better way for verification
> now, current assertion is good enough for me. Let's land this fix now and
> focus on other intermittent issues.

Thanks for review! I wish we had better and easier way to fix this :/ But you're right let's return all our integration tests back first.

Master: https://github.com/mozilla-b2g/gaia/commit/4cdac959cf4838dc5ea14f75b16d983182f7a02a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.