Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Acceptance tests and YouTube #3446

Closed
valera-rozuvan opened this issue Apr 23, 2014 · 26 comments
Closed

Acceptance tests and YouTube #3446

valera-rozuvan opened this issue Apr 23, 2014 · 26 comments
Assignees

Comments

@valera-rozuvan
Copy link
Contributor

Introduction

As everyone is aware, currently we have a very bad situation with flaky Video player acceptance tests. One of the main reasons why the tests are flaky - they rely on an external resource - YouTube. From a developer's point of view, YouTube consists of three parts:

  1. YouTube metadata server - reports back simple information about a video (duration, etc.).
  2. YouTube JavaScript Player API - the JavaScript part that runs in-browser (provides methods to play the video, etc.).
  3. Video server - serves video stream to Flash player or IFrame-based HTML5 player.

What can be done?

Several things have been discussed:

  1. Running Video acceptance tests separately from all of the other acceptance tests. Run the tests on demand, and not automatically.
  2. Creating a minimal YouTube clone. It can be automatically updated (synced) with actual YouTube on a regular basis.
  3. Adding a "before all tests" step which will check if YouTube is available (all three parts). If it is not available, then all of the Video acceptance tests will be turned off for the current run.

Discussion

I think this issue is affecting all of the developers at edX. Because of the flaky tests it sometimes takes a couple of Jenkins runs to get a passing build. This wastes time. Please contribute to this discussion! I believe that this technical issue can be solved. It is just a matter of choosing the most efficient strategy.

@valera-rozuvan
Copy link
Contributor Author

@singingwolfboy
Copy link
Contributor

I think we should mock out the Youtube API -- it's the only way to have stable tests, and to make sure that all of our tests run on every build. We'll have to keep our mock API in sync with the real API, but hopefully that won't be too difficult.

@valera-rozuvan
Copy link
Contributor Author

@singingwolfboy Please remember - we will have to also mock the other 2 parts - serving video server, and metadata server.

@singingwolfboy
Copy link
Contributor

@valera-rozuvan Do you have an estimate for how much work that will entail?

@valera-rozuvan
Copy link
Contributor Author

@singingwolfboy Unfortunately, YouTube is closed source. Their player, and their protocol - is closed source. We will have to reverse engineer A LOT of stuff. Unless Google will want to help out...

@cpennington
Copy link
Contributor

Unless we can abstract the stuff that's youtube out from the stuff that's our video player. And just test our own code. still leaves us with having the abstraction layer being untested.

@cpennington
Copy link
Contributor

Basically, we don't get much value from the parts of tests that actually hit youtube. The places the tests add value are when they are verifying code that we've written. So, @singingwolfboy's suggestion of mocking the parts of the youtube api that we use makes sense to me. In particular, if we know the places that we're using the youtube api, then we can simply mock out those pieces of the api, and return the values that we expect.

@cpennington
Copy link
Contributor

We wouldn't have to reverse engineer the youtube player, we'd just need to know about the places where we already hook into it.

@valera-rozuvan
Copy link
Contributor Author

@cpennington For JavaScript unit tests - we have completely mocked out YouTube API. Please see https://github.com/edx/edx-platform/blob/master/common/lib/xmodule/xmodule/js/spec/helper.js#L4 . What we need to fix is the acceptance tests.

@valera-rozuvan
Copy link
Contributor Author

@cpennington Currently - there is no open-source JavaScript player that can play a YouTube video without using the official YouTube JavaScript Player API.

@cpennington
Copy link
Contributor

Ah, my apologies. I didn't read closely enough.

@jzoldak
Copy link
Contributor

jzoldak commented Apr 25, 2014

@valera-rozuvan - Sorry, I'm having trouble understanding which pieces are called when. Maybe a concrete example will help me.

We had a bok-choy video test failure on the test execution that gets triggered by merges to master.
See https://jenkins.testeng.edx.org/job/edx-all-tests-auto-master/SHARD=1,TEST_SUITE=unit/1617/

Looks like after 200 seconds the test still could not find the element with class is-initialized.
Which of the pieces of the YouTube system being unavailable or too slow would have caused this?

@jzoldak
Copy link
Contributor

jzoldak commented May 21, 2014

More data points from test execution results on jenkins. It seems that when it happens the slowness is not a general degradation but instead it will affect one test but not another in the same suite. See (if those results still exist on Jenkins) this example.

Here the timeout was 200 sec, and the failing test in the YouTubeVideoTest testcase class took 3 min 27 sec, but the duration of the other 12 tests was anywhere from 8.2 to 16 seconds. Obviously nowhere near the 200 sec timeout.

New idea: when a video test is going to fail due to timing out on the initialization of the YouTube player, mark it as Skip instead.

@auraz
Copy link
Contributor

auraz commented May 22, 2014

@jzoldak It seems a good start. We should be able to see number of skipped tests, and ability to run them by hand. What about to re-run it for 5 times if it fails?

Also, from your review it looks like we have different types of youtube failures
a) Single test fails due to slowness, but other work -> Skip it or Re-run it?
b) All tests fails due to youtube failure -> we should have a single test that checks that all 3 youtube services are working.

What you think if suggest before every test we will check for youtube availability? If it unavailable then we can safely skip test. By that we will distinguish failures in youtube and in our code.

@jzoldak
Copy link
Contributor

jzoldak commented May 22, 2014

@muhammad-ammar had another good suggestion which was to look into the flaky nose plug-in.

That would allow for re-running (rather than skipping) video tests that fail intermittently. (Condition A above).

For Condition B - I like that idea also. We can add to the setup method for the video tests that need the youtube services (not stubbed): check that the services are running and if they are not, then skip them.

@auraz
Copy link
Contributor

auraz commented May 22, 2014

I like that for A!
For B -> I agree, but rather before each test than before all of them.

@cpennington
Copy link
Contributor

I'd love to see a decorator to mark the tests that depend on youtube, that either checks once and caches the result, or checks before every test.

@cpennington
Copy link
Contributor

(Or it could be even more sophisticated, and check before each test, but if it fails n times in a row, skip all the rest of the tests, or something like that).

@auraz
Copy link
Contributor

auraz commented May 22, 2014

Decorator seems fine for me.

@jzoldak
Copy link
Contributor

jzoldak commented Jun 2, 2014

We have now merged in a number of PRs to implement the above-discussed features.

As it is now, I have seen a failure in master where the youtube availability check before each bok-choy test came back good, but still flaky kicked in and the test failed twice (once marked skipped and then marked FAILED).

19:09:43 test_video_language_menu_working (acceptance.tests.video.test_video_module.YouTubeVideoTest) ... #21 passed
19:09:57 test_video_position_stored_correctly_with_seek (acceptance.tests.video.test_video_module.YouTubeVideoTest) ... #22 skipped
19:10:11 test_video_position_stored_correctly_with_seek (acceptance.tests.video.test_video_module.YouTubeVideoTest) ... #22 FAILED
19:10:26 test_video_position_stored_correctly_wo_seek (acceptance.tests.video.test_video_module.YouTubeVideoTest) ... #23 passed

@muhammad-ammar
Copy link
Contributor

Thanks for the Update.
Regarding test_video_position_stored_correctly_with_seek it needs to be improved.

@auraz
Copy link
Contributor

auraz commented Jun 3, 2014

Blades team will have a meeting tomorrow about this and we will discuss it.

@auraz
Copy link
Contributor

auraz commented Jun 4, 2014

We think this is ok for now. Let's see what happens along the way.

@jzoldak
Copy link
Contributor

jzoldak commented Jul 14, 2014

@auraz unfo this continues to be a problem and a drain on the testeng team (answering questions about failed builds) and other teams (rerunning builds trying to get a "clean" build).
I'm also suspecting that the root cause is not just youtube response time or traffic, as I have now seen the same symptoms when taking an edX course from home on a crappy wifi network (the spinner never goes away and the video never fully loads unless/until you refresh the browser). :(

@polesye
Copy link
Contributor

polesye commented Jul 15, 2014

Thanks for the information. We really need to investigate such a problem on slow networks. For now, I've created a ticket (BLD-1192) and put it into Blades backlog. If you want to speed up these things, please contact with @explorerleslie

@jzoldak
Copy link
Contributor

jzoldak commented Aug 1, 2014

@auraz @polesye I believe I might have captured enough info on https://openedx.atlassian.net/browse/BLD-1194 to have someone work on a potential fix. We should prioritize this, as the video test failures are especially bad right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants