Skip to content

Conversation

@polesye
Copy link
Contributor

@polesye polesye commented Oct 14, 2013

This PR provide fix for disabled video tests.
After enabling some of the JavaScript unit tests works well.
Some tests are fixed.
One test is rewritten to acceptance test.

@wedaly @valera-rozuvan @jzoldak please review.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you added commented-out tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was commented out before @valera-rozuvan did a fix for Video player. It will be enabled before merge.

@valera-rozuvan
Copy link
Contributor

@polesye Locally, on my machine, all Jasmine tests pass. I tried both test:js:run[xmodule] and test:js:dev[xmodule]. However, one acceptance test is falling: https://gist.github.com/valera-rozuvan/ddb095700e3e81140dac . I believe that you already have mentioned a fix for this https://github.com/edx/edx-platform/blob/86275272690d2cf1f9932217963297cf52c9b1fe/cms/djangoapps/contentstore/features/component_settings_editor_helpers.py#L39 . This fix will come in to master with the PR https://github.com/edx/edx-platform/pull/1344 .

So from me 👍 Good to merge.

@ghost ghost assigned wedaly Oct 16, 2013
@jzoldak
Copy link
Contributor

jzoldak commented Oct 16, 2013

Looks good to me, though you might want to first rebase to get my refactoring of cms acceptance tests in PR #1370 . That PR will also speed up the video .feature tests because the course/section/subsection are now created programmatically.

👍

@polesye
Copy link
Contributor Author

polesye commented Oct 17, 2013

Rebased.

@wedaly
Copy link
Contributor

wedaly commented Oct 17, 2013

I'm getting some failures locally with the caption acceptance tests. In particular, the "Make sure captions are open" step fails intermittently with a WebDriverException:

"unknown error: Element is not clickable at point (314, 639). Other element would receive the click"

Make sure you're using the ui helper functions in common/djangoapps/terrain/ui_helpers. In particular, the css_click method has retry logic for exactly this kind of exception.

@jzoldak
Copy link
Contributor

jzoldak commented Oct 17, 2013

@polesye you might want to also get the small change I made today for better synchronization when adding units with the create_component_instance method. It's in PR #1394

@polesye
Copy link
Contributor Author

polesye commented Oct 18, 2013

@wedaly Some fixes are provided by PR #1394. This branch is rebased. Please check again.

@wedaly
Copy link
Contributor

wedaly commented Oct 18, 2013

Ran the acceptance tests a bunch of times and they seem solid. Thanks for taking care of this!

👍

polesye added a commit that referenced this pull request Oct 18, 2013
@polesye polesye merged commit 70e5e80 into master Oct 18, 2013
@polesye polesye deleted the anton/fix-video-js-unit-tests branch October 18, 2013 18:45
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Oct 27, 2016
giovannicimolin pushed a commit to open-craft/openedx-platform that referenced this pull request Jan 15, 2019
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

Successfully merging this pull request may close these issues.

6 participants