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

Jonahstanley/add courseteam tests #140

Merged
merged 23 commits into from
Jun 21, 2013

Conversation

JonahStanley
Copy link
Contributor

Suggested reviewers: Studio team dev, @jzoldak
In these files, I have added the following testing features:

  1. CMS tests now pull from their own contentstore database. This mirrors the behavior of how cms interacts with the modulestore databse and allows for it to be refreshed upon each scenario.
  2. Upload
    This tests the file uploading page. These tests mostly cover uploading a file and making sure if the same file is uploaded twice, it only appears once. At the moment, it was not feasible to be able to download a file, access it, and clean up at the end during the test. This would allow for making sure the download / update worked by checking the hash of the file.
  3. Course Team
    The ability to add and remove other users to the course team was tested. When another user is added, they can then access the course in their studio. Also, it is not possible to add a user who does not exist. Lastly, the added user is not able to add or delete other users. I also changed the common.py such that the course name/org/number are constants that can be changed at the top of the file and also allows for other tests to import those names.
  4. Static Pages
    The ability to add, delete and edit static pages was tested. Also, ostensibly, the sortability was tested. As of this pull request, selenium does not interact well with jQuery Sortable items. Thus, I had to introduce a somewhat hack in order to at least gain the visual affects of sorting. However, it is very fragile upon refreshing the page, the order does not necessarily propagate. This is due to the amount of events that sorting fires during the moving process and the difficulty in manually firing all of the dependent events. This method was also added to the world such that it could be possible to test reordering in other places. It takes the css class of the items, the 2 indicies, and the css class of the sortable table/thing.
  5. Course Updates/Handouts
    The ability to add/delete/edit course updates was tested as well as the ability to update the dates of the updates and the ability to change the handouts.

Hopefully, with these new testing features, every page of studio is now tested. The only known thing that isn't tested is drag and drop / sortability of subsections and sections. This is due to the aforementioned issue with drag and drop as well as when I manually tested it, it was a bit buggy.

JonahStanley added 6 commits June 10, 2013 14:38
Also modified the create_course method in common to allow for the possibility to access the course name and etc. from other files.
test.py was changed such that the database being used is test_xcontent
course_helpers.clear_courses now calls contentstore().fs_files.drop() such that all content is removed and the databse is started afresh in the same manner as the modulestore
TODO: When deleting is available, test deleting uploaded files`
Also added in drag and drop helper.  Please be careful using this method as it is truly a hack to obtain the visual effects.  As of this commit, selenium does not support drag and drop for jQuery sortable items
@JonahStanley
Copy link
Contributor Author

Also, when deleting an uploaded file becomes available (might be done this sprint), that scenario can be added to the upload tests


# Add the user to the instructor group of the course
# so they will have the permissions to see it in studio
g = world.GroupFactory.create(name='instructor_MITx/999/Robot_Super_Course')
g = world.GroupFactory.create(name='instructor_MITx/%s/%s' % (COURSE_NUM, COURSE_NAME.replace(" ", "_"),))
Copy link
Contributor

Choose a reason for hiding this comment

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

Small style issue: for new code, using format() instead of % is preferred:
http://docs.python.org/2/library/string.html#format-examples

@wedaly
Copy link
Contributor

wedaly commented Jun 12, 2013

@JonahStanley I'm very excited that these features are getting covered by your tests! A few things, from most to least important:

  1. I'm concerned about the call to os.getcwd(). There are more reliable ways to get a path to fixture files.

  2. Let's make absolutely sure that dragging helpers work across environments, 100% of the time. I'd hate to make the test suite flakey again after all your hard work fixing flakey tests!

  3. BDD-style specs should avoid referencing UI elements. The UI is an implementation detail that could change; the expected behavior of the system (adding updates, uploading files, adding team members) is usually more stable.

There were a few other style notes, but those were the main things. Let me know when you're ready for me to take another look.

JonahStanley added 4 commits June 12, 2013 13:25
% to format
css_find().click() to its actual function
A few name changes as well as step changes
Removal of drag testing
A refactoring of login and create user.  Login will no longer create the specified user
A few notes:
1. Downloads are done through direct requests.  This is due to the difficulty of downloading a file to the correct place
2. Modifiying a file will just change the file to a random 10 character string.
3. The page is reloaded in between uploads.  This is due to a current caching bug that is in the process of being looked into and will be updated once it is fixed.
@JonahStanley
Copy link
Contributor Author

I am having trouble with reverting the changes I do to the test file after the scenario runs

file_css = '.file-input'
upload = world.css_find(file_css)
#uploading the file itself
upload._element.send_keys(TEST_ROOT + '/uploads/' + file_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use os.path.join() here to guarantee that the paths are combined correctly, without needing to specify forward slashes explicitly.

@wedaly
Copy link
Contributor

wedaly commented Jun 17, 2013

@JonahStanley About reverting the changes to the uploaded file:

The rakefile resets the test_root directory when the rake task runs. As long as no other test uses the fixture (which is a reasonable assumption since you've put it in an uploads directory), this should be okay. To make the assumption even more explicit, you can add a line in the uploads/test that says something like, "This file will be modified during the test run and should not be used except by uploads.feature"

@wedaly
Copy link
Contributor

wedaly commented Jun 17, 2013

I tried running the tests locally, and I'm getting some failures for upload.py. Here's one example (the tracebacks are all the same):

Feature: Upload Files # cms/djangoapps/contentstore/features/upload.feature:1
As a course author, I want to be able to upload files for my students # cms/djangoapps/contentstore/features/upload.feature:2
Installed 0 object(s) from 0 fixture(s)

Scenario: Users can upload files # cms/djangoapps/contentstore/features/upload.feature:4
Given I have opened a new course in Studio # cms/djangoapps/contentstore/features/common.py:54
And I go to the files and uploads page # cms/djangoapps/contentstore/features/upload.py:15
When I upload the file "test" # cms/djangoapps/contentstore/features/upload.py:23
Traceback (most recent call last):
File "/Users/will/.virtualenvs/edx-platform/lib/python2.7/site-packages/lettuce/core.py", line 143, in call
ret = self.function(self.step, _args, *_kw)
File "/Users/will/edx_all/edx-platform/cms/djangoapps/contentstore/features/upload.py", line 30, in upload_file
upload._element.send_keys(TEST_ROOT + '/uploads/' + file_name)
File "/Users/will/.virtualenvs/edx-platform/lib/python2.7/site-packages/selenium/webdriver/remote/webelement.py", line 159, in send_keys
self._execute(Command.SEND_KEYS_TO_ELEMENT, {'value': typing})
File "/Users/will/.virtualenvs/edx-platform/lib/python2.7/site-packages/selenium/webdriver/remote/webelement.py", line 225, in _execute
return self._parent.execute(command, params)
File "/Users/will/.virtualenvs/edx-platform/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 160, in execute
self.error_handler.check_response(response)
File "/Users/will/.virtualenvs/edx-platform/lib/python2.7/site-packages/selenium/webdriver/remote/errorhandler.py", line 149, in check_response
raise exception_class(message, screen, stacktrace)
ElementNotVisibleException: Message: u'Element must be displayed to click'
Then I see the file "test" was uploaded # cms/djangoapps/contentstore/features/upload.py:37
And The url for the file "test" is valid # cms/djangoapps/contentstore/features/upload.py:43
Installed 0 object(s) from 0 fixture(s)

@JonahStanley
Copy link
Contributor Author

@chrisndodge I added tests for uploading/downloading files. Is there anything I am forgetting to test or does it look good to you?

Also, in one of your previous commits, you changed the name of the contentstore database to be named test_xmodule. It also seemed like the unit tests you wrote rely on that name. Would it be possible for you to change it so they aren't name dependant (as it should be named test_xcontent)? If you want to see which unit tests failed, you can see them in the failed builds. Thanks so much.

@chrisndodge
Copy link
Contributor

I'll take a look in the AM tomorrow. However, I'm not super familiar with the UI testing frameworks - I mainly work in the unit test arena.

I'm not 100% sure by what you mean "the unit tests you wrote rely on that [test_xmodule] name"? Do you mean in the test.py configuration? But I don't see any code that explicitly refers to that database name, or am I missing something. I can see wanting to rename that to test_xcontent, so I'm happy to do that.

While I'm looking at this code, I think I need to ping @jzoldak and make sure we're deleting the xcontent database as well between unit test runs. I'm not sure we are.

@JonahStanley
Copy link
Contributor Author

The only reason I ask about that is because the unit tests seem to fail on jenkins when the database is named test_xcontent and not test_xmodule (at least since I merged in master). I was more wondering if you knew why that could be the case. I am in the process of also making sure the test_xcontent / test_xmodule databases are properly cleaned after runs so I can talk to you about that later as well

@chrisndodge
Copy link
Contributor

Hi Jonah,

OK, I looked at the failed build and I see a "corrupted GridFile" in the test output. I'd be curious to hear what happens if you set it to test_xcontent and clean up the DB between runs. Also, could there be a concurrency issue happening right now, e.g. multiple files with the same name being uploaded at the same time?!?

Conflicts:
	cms/djangoapps/contentstore/features/common.py
@jzoldak
Copy link
Contributor

jzoldak commented Jun 20, 2013

@JonahStanley can you pls clean up the pep8 and pylint violations in these files?

@@ -13,6 +13,10 @@
from logging import getLogger
logger = getLogger(__name__)

COURSE_NAME = 'Robot Super Course'
COURSE_NUM = '999'
COURSE_ORG = 'MITx'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please prefix these variable names with an underscore.

@chrisndodge
Copy link
Contributor

Did you want to get this PR to also switch over the xcontent database name to be distinct from the xmodule database, ala what we talked about earlier today? It's not a requirement for me - but it seemed to be your original intent.

Otherwise I'm fine with this PR.

@step(u'I go to the course updates page')
def go_to_uploads(step):
menu_css = 'li.nav-course-courseware'
uploads_css = '.nav-course-courseware-updates'
Copy link
Contributor

Choose a reason for hiding this comment

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

For this, and all the other elements that we will be finding via css in this file, please use the element type as well as the class, instead of just the class.

@jzoldak
Copy link
Contributor

jzoldak commented Jun 20, 2013

👍

Because it bothers me, although I don't expect anyone else to care.
@singingwolfboy
Copy link
Contributor

👍

JonahStanley pushed a commit that referenced this pull request Jun 21, 2013
@JonahStanley JonahStanley merged commit 26a05f2 into master Jun 21, 2013
@JonahStanley JonahStanley deleted the jonahstanley/add-courseteam-tests branch June 21, 2013 19:43
chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
aboudreault pushed a commit to aboudreault/edx-platform that referenced this pull request Jul 30, 2014
jbau pushed a commit that referenced this pull request Jan 21, 2015
diegomillan referenced this pull request in eduNEXT/edx-platform Sep 14, 2016
* stv/image-modal/aria:
  Upgrade ImageModal XBlock
xavierchan added a commit to xavierchan/edx-platform-1 that referenced this pull request Dec 24, 2018
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.

5 participants