Closed
Bug 988516
Opened 11 years ago
Closed 10 years ago
[Calendar] Intermittent failing test, day view events longer than 2h click after first hour
Categories
(Firefox OS Graveyard :: Gaia::Calendar, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.0 S5 (4july)
People
(Reporter: kgrandon, Assigned: evanxd)
References
Details
(Whiteboard: [p=1])
Attachments
(2 files)
1) day view events longer than 2h click after first hour:
title should match: expected '' to equal 'Lorem Ipsum'
Reporter | ||
Comment 1•11 years ago
|
||
Keywords: leave-open
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → evanxd
Assignee | ||
Comment 3•11 years ago
|
||
Comment on attachment 8405775 [details]
Pull request
Hi Kevin,
Could you have time to help me to review the patch?
And I think it is a same issue of Bug 974731.
The re-enable test is passed for 204 times continually on Travis, see it at https://travis-ci.org/mozilla-b2g/gaia/builds/22845869 and https://travis-ci.org/mozilla-b2g/gaia/builds/22845792.
Thanks.
Attachment #8405775 -
Flags: review?(kgrandon)
Reporter | ||
Comment 4•11 years ago
|
||
Comment on attachment 8405775 [details]
Pull request
We can try this and see how it does. Thanks!
Attachment #8405775 -
Flags: review?(kgrandon) → review+
Reporter | ||
Comment 5•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 6•11 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/d7af06cc5bc0372355d420e54e3c67a965719eae this seems misplaced. We just got done with a [challenging] integration test rewrite because this thing https://github.com/mozilla-b2g/gaia/commit/d920928de2cfc64e373c22ad1ab090173bf5e193 got really clunky. We have to try [incrementally] on a per-patch basis to keep up code quality to avoid that situation again. The library structure only gets us so far. We must also be vigilant in making sure that we're using our abstractions reasonably.
It is not an explicit part of the view contract that all properties will return elements' text content. This is confusing and I do not support this changeset.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 7•11 years ago
|
||
https://github.com/gaye/gaia/commit/e0985cfba03c18adc12e55a0560192aae76c3fdf reverted. :evanxd please do take another look at this. I would like to get the test re-enabled but not at the expense of our test code quality.
Reporter | ||
Comment 8•11 years ago
|
||
Thanks Gareth. Some of the abstraction style did feel a bit off for me as well, but I guess I was too happy to enable another test :)
I will be sure to forward any reviews to you if I have doubt. Thanks!
Comment 9•11 years ago
|
||
Believe me I want more tests turned on too. I just don't want to tell the product team that we have to take another few weeks for integration test refactoring anytime in the near future. I don't think my credibility would survive that lol
Assignee | ||
Comment 10•11 years ago
|
||
Hi Gareth and Kevin,
Sorry for that.
I just would like to give it a try to re-enable the tests.
And Miller and I also discussed a good way to fix the race condition issue with good quality code, see it at https://github.com/mozilla-b2g/gaia/pull/18256#discussion-diff-11560433.
Assignee | ||
Comment 11•11 years ago
|
||
Hi Gareth,
So do yxu have any suggestion to fix this issue?
We could discuss that at https://github.com/mozilla-b2g/gaia/pull/18256#discussion-diff-11560433.
Let's re-enable the disable tests.
Thanks.
Flags: needinfo?(gaye)
Assignee | ||
Comment 12•11 years ago
|
||
We could follow https://github.com/mozilla-b2g/gaia/pull/18256/files#r11649532 to fix this bug.
Comment 13•11 years ago
|
||
Evan, I did a quick implementation on the week view visual refresh. I overwrote the `waitForDisplay` on the `readEvent` view, which should fix the intermittent failures and is a cleaner implementation than pooling for the text content at each assertion.
The main problem with the old implementation was that we would probably write more tests in the future that would also cause intermittent failures simply because we forgot to call `waitForTextNotEmpty()`... the duplication of `waitForTextNotEmpty` calls everywhere was a big red flag, the main reason why we decided to create the "view" abstractions was to avoid duplication and to encapsulate all the weirdness into a single/common place.
This is what I ended up using:
waitForDisplay: function() {
// event details are loaded asynchronously, so we need to wait until the
// "loading" class is removed from the view; this will avoid intermittent
// failures on Travis (caused by race condition)
var element = this.getElement();
this.client.waitFor(function(){
return element.displayed() &&
element.getAttribute('class').indexOf('loading') === -1;
});
},
week view should land pretty soon, so let's see if it also fixes this problem... in the meantime, if you could test my proposed change that would be really helpful.
Depends on: 951071
Flags: needinfo?(gaye) → needinfo?(evanxd)
Assignee | ||
Comment 15•10 years ago
|
||
The test works well now.
It could be passed on try server for 20 times[1] continually.
[1] https://tbpl.mozilla.org/?rev=9577d31c019d9e1ac7246d0b4dc7996a033fdf8f&tree=Gaia-Try
Assignee | ||
Comment 16•10 years ago
|
||
Let's re-enable the test.
Assignee | ||
Comment 17•10 years ago
|
||
master: 3cdfce6abbebf5bdf3d4ddec06cb6ccc581729ae
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•10 years ago
|
Whiteboard: [p=1]
Target Milestone: --- → 2.1 S5 (26sep)
Updated•10 years ago
|
Target Milestone: 2.1 S5 (26sep) → 2.0 S5 (4july)
Comment 18•7 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•