Skip to content

Ihoover/automated content testing - #493

Closed
ihoover wants to merge 4 commits into
masterfrom
ihoover/automated_content_testing
Closed

Ihoover/automated content testing#493
ihoover wants to merge 4 commits into
masterfrom
ihoover/automated_content_testing

Conversation

@ihoover

@ihoover ihoover commented Jul 24, 2013

Copy link
Copy Markdown
Contributor

Framework for course authors to create tests for the problems they author. This PR has just the framework laid, and the purpose is to get this code reviewed before moving on. All urls associated are disabled by the feature flag MITX_FEATURES['content_testing'].

To use this code, first go to cms/envs/common.py and modify line 64 to read 'CONTENT_TESTING': True,. This enables the url paths /test_problem/.... To use the code, run rake cms and navigate to /test_problem/?location=<location>, where <location> is the location of a problem you have the authorization to edit.
@jzoldak @wedaly

@wedaly

wedaly commented Jul 31, 2013

Copy link
Copy Markdown
Contributor

I know this is a work-in-progress, so I'll start with some high-level comments and questions:

  1. The hashing/rematching scheme is clever. However, there are many permutations of create/delete/update operations you can do on elements of the problem XML, and I'm worried there might be edge-cases there that are missing. Let's meet sometime this week, and we can talk through how to chart these out.

  2. I'm concerned about the use of the global cache variable. This guarantees that the models are not thread-safe, which has the potential to create some nasty bugs. I'd like to hear more about why you made that choice and explore some alternatives.

  3. I like that the content-testing logic is encapsulated in a single Django app. Even better, it's encapsulated in Django models. The trade-off is that you're now responsible for handling course changes gracefully.

Some edge cases to consider (there are probably more of them):

  • What happens when a problem gets deleted from a course and I run the tests?
  • What if I deleted the problem by mistake, so I recreate it from a backup course I exported? Can I easily update the test to point to the new problem? Or do I have to recreate the test cases as well?
  • What if I change the problem XML to something invalid, so that you can't load the problem at all?
  1. I agree that the regex HTML manipulation is a hack. I need to hear more about why this is necessary to think about viable alternatives.

In terms of low-level style feedback, I'll hold off until this is closer to ready. Overall, though, your code has gotten much cleaner. From your comments, I think you're aware of which parts are still messy :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add comments explaining why you chose this specific formula. What desirable properties does it have?

@wedaly

wedaly commented Aug 23, 2013

Copy link
Copy Markdown
Contributor

This is almost ready to merge, just a few last things:

  1. Please look through the code carefully and fix style violations. You need to always use double-quote docstrings, capitalize the first letter of docstrings, and look out for typos.

  2. You could add to the comments in a few places so that people less familiar with the code can more easily see what it's doing. See my line notes for suggestions.

  3. The views could use more error handling/validation. However, I know that the views are the part of this most likely to change as it gets integrated to Studio -- so it's probably a better use of time to document the exceptions that the CRUD operations can throw (invalid location, etc.).

  4. One of your test cases will cause parallelization issues on Jenkins -- make sure you override settings to use a unique Mongo collection.

@wedaly

wedaly commented Aug 23, 2013

Copy link
Copy Markdown
Contributor

Also, what is the status of the Draft modulestore issue you mentioned? Even if it's not fixed in this PR, I'd like to have it documented in a wiki page as a known issue.

@singingwolfboy

Copy link
Copy Markdown
Contributor

What's the status of this pull request?

@wedaly

wedaly commented Sep 9, 2013

Copy link
Copy Markdown
Contributor

@singingwolfboy Working on getting it merged; the last two weeks have just been crazy.

@singingwolfboy

Copy link
Copy Markdown
Contributor

This pull request has gone well and truly stale. I'm closing it for now, but I'm not deleting the branch; we can come back to it when we have time.

chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
@benpatterson
benpatterson deleted the ihoover/automated_content_testing branch January 21, 2015 13:14
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.

3 participants