-
Notifications
You must be signed in to change notification settings - Fork 73
Conversation
6971872
to
07851f0
Compare
Codecov Report
@@ Coverage Diff @@
## master #170 +/- ##
==========================================
+ Coverage 97.85% 97.85% +<.01%
==========================================
Files 54 54
Lines 3119 3129 +10
Branches 234 236 +2
==========================================
+ Hits 3052 3062 +10
Misses 42 42
Partials 25 25
Continue to review full report at Codecov.
|
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.
Yep! It really is just about that simple. We typically want to try to add some local "fake" data for testing too. You'll see an example at https://github.com/edx/edx-analytics-data-api/blob/master/analytics_data_api/management/commands/generate_fake_course_data.py#L167.
Thanks!
I just realized that we probably want to ensure that the |
No problem, I'll get that added this afternoon |
count = int(ratio * daily_total) | ||
pass_rate = min(random.normalvariate(.45 + (.1 * index), .15), 1.0) |
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.
I utilize index
here so we can get a higher pass rate for verified
than we have for audit
(scales with position in `enrollment_mode_ratios); is that overly complex for this data?
I also just guessed at the baseline pass rates (from 45% for audit up to 85% for verified), LMK if there's a more accurate value we ought to use.
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.
❤️
self.generate_data() | ||
response = self.authenticated_get(self.path(exclude=[field])) | ||
self.assertEquals(response.status_code, 200) | ||
self.assertEquals(str(response.data).count(field), 0) |
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.
self.assertNotIn(field, response.data)
only works for the top-level (cumulative) field, not the per-mode field. This ensures the field is not present anywhere.
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.
Nice!
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.
LGTM! Don't forget to add your name to AUTHORS though. 💯
count = int(ratio * daily_total) | ||
pass_rate = min(random.normalvariate(.45 + (.1 * index), .15), 1.0) |
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.
❤️
self.generate_data() | ||
response = self.authenticated_get(self.path(exclude=[field])) | ||
self.assertEquals(response.status_code, 200) | ||
self.assertEquals(str(response.data).count(field), 0) |
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.
Nice!
@@ -120,6 +121,10 @@ def postprocess_field_dict(self, field_dict): | |||
# don't do expensive looping for programs if we are just going to throw it away | |||
field_dict = self.add_programs(field_dict) | |||
|
|||
for field in self.exclude: |
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.
Thanks for taking care of that!
bdc0c95
to
ea49b2b
Compare
ea49b2b
to
09c951f
Compare
EDU-153
This feels too easy - am I missing something?