Closed MichaelRoytman closed 4 years ago
looks good. I think we should add a test case that creates an enrollment input with extra fields just to cover the new case. If you're planning to refactor this to be used by the learner report improvement as well then we can probably just hit it with that PR instead.
@Zacharis278 Thanks for the feedback. I tried to do that originally re: testing, but because the output of both the mapper
and reducer
functions hasn't changed, it doesn't really matter what the enrollment input is; with or without this change, the output is the same. I could test _trim_fields
by directly calling it, but I figured this was an implementation detail that doesn't affect the output of the task; it just prevents an exception. The test of whether this change works is whether it throws an exception. What do you think?
Merging #810 into master will increase coverage by
<.01%
. The diff coverage is90%
.
@@ Coverage Diff @@
## master #810 +/- ##
==========================================
+ Coverage 74.37% 74.37% +<.01%
==========================================
Files 211 211
Lines 24399 24407 +8
==========================================
+ Hits 18146 18153 +7
- Misses 6253 6254 +1
Impacted Files | Coverage Δ | |
---|---|---|
edx/analytics/tasks/common/vertica_export.py | 40.87% <ø> (ø) |
:arrow_up: |
...ytics/tasks/programs/tests/test_program_reports.py | 100% <ø> (ø) |
:arrow_up: |
edx/analytics/tasks/programs/program_reports.py | 93.65% <90%> (-0.19%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update e632920...be77799. Read the comment docs.
Analytics Pipeline Pull Request
Make sure that the following steps are done before merging: