Kevin Grandon :kgrandon Reporter
	Description • 10 years ago

1) day view events longer than 2h click after first hour:

title should match: expected '' to equal 'Lorem Ipsum'

	Kevin Grandon :kgrandon Reporter
	Comment 1 • 10 years ago

Disabled: https://github.com/mozilla-b2g/gaia/commit/63825af8c121827cccf4aabfec00639a6b3a2ebe

	Evan Tseng [:evanxd] Assignee
	Comment 3 • 10 years ago

Comment on attachment 8405775 [details] [review]
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.

	Kevin Grandon :kgrandon Reporter
	Comment 4 • 10 years ago

Comment on attachment 8405775 [details] [review]
Pull request

We can try this and see how it does. Thanks!

	Kevin Grandon :kgrandon Reporter
	Comment 5 • 10 years ago

Landed: https://github.com/mozilla-b2g/gaia/commit/28933e686babd4d95b37e6056e4febc0480ff759

	Gareth Aye [:gaye] (back from PTO)
	Comment 6 • 10 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.

	Gareth Aye [:gaye] (back from PTO)
	Comment 7 • 10 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.

	Kevin Grandon :kgrandon Reporter
	Comment 8 • 10 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!

	Gareth Aye [:gaye] (back from PTO)
	Comment 9 • 10 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

	Evan Tseng [:evanxd] Assignee
	Comment 10 • 10 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.

	Evan Tseng [:evanxd] Assignee
	Comment 11 • 10 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.

	Evan Tseng [:evanxd] Assignee
	Comment 12 • 10 years ago

We could follow https://github.com/mozilla-b2g/gaia/pull/18256/files#r11649532 to fix this bug.

	Miller Medeiros [:millermedeiros]
	Comment 13 • 10 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.

	Evan Tseng [:evanxd] Assignee
	Comment 14 • 10 years ago

Sure, let's test your work.

	Evan Tseng [: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

	Evan Tseng [:evanxd] Assignee
	Comment 16 • 10 years ago

Let's re-enable the test.

	Evan Tseng [:evanxd] Assignee
	Comment 17 • 10 years ago

master: 3cdfce6abbebf5bdf3d4ddec06cb6ccc581729ae

	Firefox Bug Husbandry Bot
	Comment 18 • 6 years ago

Removing leave-open keyword from resolved bugs, per :sylvestre.