1219704 - Implement *test_play_ogg_video.py* as an integration test in JavaScript

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Video, defect)

Description

[William Hsu [:whsu]]

https://github.com/mozilla-b2g/gaia/tree/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/videoplayer/test_play_ogg_video.py

Comment 1

[Russ Nicoletti [:russn]]

Hi Punam, I'm having an issue 'clicking' on a thumbnail of a video in order to play it. The test is crashing when 'click' is invoked. I have updated lib/video.js to contain the same functions as lib/gallery.js in terms of clicking on a thumbnail. I am invoking 'tapThumbnails(n)' [1] the same as gallery tests [2]. My test crashes, the gallery test succeeds. Can you have a look? 

[1] https://github.com/russnicoletti/gaia/blob/bug-1219704/apps/video/test/marionette/open_video_test.js#L34
    https://github.com/russnicoletti/gaia/blob/bug-1219704/apps/video/test/marionette/lib/video.js#L158-L163

[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/test/marionette/load_valid_image_test.js#L49
    https://github.com/russnicoletti/gaia/blob/bug-1219704/apps/gallery/test/marionette/lib/gallery.js#L129-L134

Comment 2

[Russ Nicoletti [:russn]]

More information. In lib/video.js, when I change:

thumbnails[n].click();

to:

var thumbnail = thumbnails[n];
thumbnail.click();

I get the following error: 

'ElementNotAccessibleError: Element is not focusable via the accessibility API -> id: , tagName: LI, className: thumbnail\n\nRemote Stack:\n<none>'

Comment 3

[Punam Dahiya [:pdahiya]]

Hi Russ, Looking at the code I don't see anything missing. I tried running video test on my local and able to replicate the crash. There is no useful error shown in logs , I have tried with VERBOSE=1 as well.

DOM to create and display list of thumbnail is slightly different in video than gallery, I think selectors in video might need to be updated to select video thumbnail item. 

If you manually click video thumbnail while running test, it does show video playing, I will debug some more and keep you posted. Thanks!

Comment 4

[Punam Dahiya [:pdahiya]]

(In reply to Russ Nicoletti [:russn] from comment #2)
> More information. In lib/video.js, when I change:
> 
> thumbnails[n].click();
> 
> to:
> 
> var thumbnail = thumbnails[n];
> thumbnail.click();
> 
> I get the following error: 
> 
> 'ElementNotAccessibleError: Element is not focusable via the accessibility
> API -> id: , tagName: LI, className: thumbnail\n\nRemote Stack:\n<none>'

 ok, that was it, removing raisesAccessibilityExceptions: true let test pass. Looks like accessibility API is throwing exception and crash when thumbnaiitem is clicked.

https://github.com/russnicoletti/gaia/blob/bug-1219704/apps/video/test/marionette/open_video_test.js#L18

Comment 5

[Russ Nicoletti [:russn]]

I was looking into why the element was not 'focusable'. I think it's because an 'li' element is not focusable by default. Adding 'tabindex=0' makes it focusable but your suggestion is cleaner. Thanks.

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] russnicoletti:bug-1219704 > mozilla-b2g:master

Comment 7

[Russ Nicoletti [:russn]]

Comment on attachment 8688079 [details] [review]
[gaia] russnicoletti:bug-1219704 > mozilla-b2g:master

Hi David, this PR is intended to resolve this bug as well as bug 1219705 and bug 1219706.

Comment 8

[Russ Nicoletti [:russn]]

Comment on attachment 8688079 [details] [review]
[gaia] russnicoletti:bug-1219704 > mozilla-b2g:master

David, I'm changing from f? to r? as discussed during 1:1.

Comment 9

[David Flanagan [:djf]]

Comment on attachment 8688079 [details] [review]
[gaia] russnicoletti:bug-1219704 > mozilla-b2g:master

Various nits on github. I think there is a way to avoid the setTimeout(), a better way to handle showControls(). Also, I think that you might be testing elapsedTime() before 1 second has elapsed, which could cause the intermittent failures.

Comment 10

[Russ Nicoletti [:russn]]

Comment on attachment 8688079 [details] [review]
[gaia] russnicoletti:bug-1219704 > mozilla-b2g:master

Hi David, I've updated the PR based on your feedback.

Comment 11

[David Flanagan [:djf]]

Comment on attachment 8688079 [details] [review]
[gaia] russnicoletti:bug-1219704 > mozilla-b2g:master

This is looking good, but I had a few nits and I think there is a bug in your waitForThumbnails() function which is likely to cause intermittent failures. If you fix the bug (or convince me that there isn't a bug) then this is pretty much an automatic r+.

Comment 12

[Russ Nicoletti [:russn]]

Comment on attachment 8688079 [details] [review]
[gaia] russnicoletti:bug-1219704 > mozilla-b2g:master

Hi David, I've updated the PR to address your latest feedback. More details in the PR.

Comment 13

[David Flanagan [:djf]]

Russ,

Please take another look at your thumbnails getter function in lib/video.js.  the `get thumbnails` getter function is exactly the same as the `getThumbnails()` function and very similar to the `get thumbnail` getter (except that one uses findElement and one uses findElements).  In all three of these cases, however, they're using 'li.thumbnail' as the selector. So I'm pretty sure that your getter function is returning an array of thumbnail elements.  If you want to get the container element you need '#thumbnails' as the selector. But that won't work either, because the thumbnails are grouped by month.

Anyway, I'm still concerned that waitForThumbnails() is broken and that thumbnails.length will never change during the waitFor() call.  Maybe I'm misunderstanding Marionette... If you can convince me that marionette's findElements() call returns a live NodeList object that updates automatically as the DOM changes then I'm okay with this code. But it doesn't look to me like it could work.

Comment 14

[David Flanagan [:djf]]

Comment on attachment 8688079 [details] [review]
[gaia] russnicoletti:bug-1219704 > mozilla-b2g:master

Clearing the review request for now. Please set it again when you've responded to my comment above.

Comment 15

[Russ Nicoletti [:russn]]

Comment on attachment 8688079 [details] [review]
[gaia] russnicoletti:bug-1219704 > mozilla-b2g:master

After more investigation, I have verified waitForThumbails is not necessary. It may be "broken" as you say but it's not necessary because after app.launch(), the thumbnails will already have been loaded so there is no need to "wait" for them. I can't point to the marionette code to prove this but I have instrumented various files and played with the timing of loading the thumbnails and I am convinced this is the case. I have removed the waitForThumbnails function and updated the test code. The PR is updated.

Comment 16

[David Flanagan [:djf]]

Comment on attachment 8688079 [details] [review]
[gaia] russnicoletti:bug-1219704 > mozilla-b2g:master

Given that the biggest problem we have with our marionette tests is intermittent errors caused by timing issues, I don't see how we can land a patch when neither of us can explain why it works. As far as we can tell, the gallery.launch() call should be returning before the thumbnails are created. I've traced this back as far as gaia/tests/jsmarionette/plugins/marionette-apps/lib/waitforapp.js and don't see anything that it making it wait any longer than it takes for the <body> element to be displayed.

So even though the test works now, we can't have any confidence that it will continue to work. I think there is working going on to make these marionette tests runnable on actual devices, and when we do that, there will be a different device storage backend, which will presumably have different timing issues. I'm guessing that maybe the device storage stub that we're getting with this test is synchronous instead of async, and would expect that if we run a device with a real device storage api, we'd start to see failures.

So I think you do need to wait for the thumbnails.

Comment 17

[Russ Nicoletti [:russn]]

Comment on attachment 8688079 [details] [review]
[gaia] russnicoletti:bug-1219704 > mozilla-b2g:master

I've re-introduced waiting for the thumbnails. The function within the 'waitFor' calls 'findElements' so whether or not a DOM node is returned or simply an array the function should behave as designed.

I also cleaned up the patch based on your other comments in the PR.

Comment 18

[David Flanagan [:djf]]

Comment on attachment 8688079 [details] [review]
[gaia] russnicoletti:bug-1219704 > mozilla-b2g:master

Looks good. Thanks, Russ.

Comment 19

[Russ Nicoletti [:russn]]

Master: https://github.com/mozilla-b2g/gaia/commit/62ad7586b05670fee9a4df256b1cc6fdb5a692e8

Comment 20

[Russ Nicoletti [:russn]]

After pulling the latest from master, which included my patch, I found I was getting an ElementNotAccessibleError when running the tests. I'm not sure why my result was different from treeherder but I created a follow up patch where I explicitly set 'raisesAccessibilityExceptions' to false in the marionette prefs.

https://github.com/mozilla-b2g/gaia/commit/2cf0778d92505fc845e962b3a2eed3ac2afa45ad