-
Notifications
You must be signed in to change notification settings - Fork 37
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
Added view tests for CoursesIndexView #7
Conversation
- Updated the mock model CourseOverview and factory code to provide both 'org' and 'display_org_with_default' and make sure they have the same value when the model instance is created
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine! but see a few comments.
] | ||
|
||
|
||
# Look into renaming 'name' to 'display_name' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you did this, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. The field names in the models may not be the same name as found in the REST API data (defined in the serializer classes, see edx_figures/serializers.py
). What I want to do is either remove the need to rename keys (which is unlikely) or find or write some key renaming class or method so I can just pass this as a parameter to model factory constructors instead of an explicit list of named parameters.
from .factories import CourseOverviewFactory | ||
|
||
# Course data and generate method are duplicates course data in test_filters.py | ||
COURSE_DATA = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe throw in some non-ASCII chars to test edge cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I'll add this when I refactor the course index test module. Created issue: #8
@@ -44,8 +44,15 @@ class CourseOverview(models.Model): | |||
id = models.CharField(db_index=True, primary_key=True, max_length=255) | |||
display_name = models.TextField(null=True) | |||
org = models.TextField(max_length=255, default='outdated_entry') | |||
# For the tests, the CourseOverviewFactory does a LazyAttribute on | |||
# display_org_with_default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to eventually test that filtering is possible via display org as well as org?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Added: #9
@bryanlandia Thanks for reviewing! |
…travis Add Juniper Tox environments to TravisCI file
'org' and 'display_org_with_default' and make sure they have the same
value when the model instance is created