Use wrap_child instead of replacing get_html

[cpennington]

@nedbat, @sarina: Review, once #909 is applied?

@wedaly: I added some test infrastructure, and made lms/cms tests add to the common/lib coverage. Can you review that?

Things to review:

1. That the wrap_child implementations do the same thing as the old get_html replacement versions
2. That the new wrappers are applied in the same places and orders
3. That there aren't any cases of replacing get_html left in the codebase

[sarina]

It'd be nice to have more comments here explaining what's going on.

[sarina]

Test coverage seems a little low. And parts of the code are a bit mind-bending - as always more comments are appreciated to help those without such an intimate knowledge of what you're working on. Otherwise I don't see any particular glaring issues.

[sarina]

-------------
Diff Coverage
Coverage Report(s) Used: /Users/sarina/edx_all/edx-platform/reports/cms/coverage.xml, /Users/sarina/edx_all/edx-platform/reports/common/lib/calc/coverage.xml, /Users/sarina/edx_all/edx-platform/reports/common/lib/capa/coverage.xml, /Users/sarina/edx_all/edx-platform/reports/common/lib/xmodule/coverage.xml, /Users/sarina/edx_all/edx-platform/reports/lms/coverage.xml
Diff: origin/master...HEAD, staged, and unstaged changes
-------------
common/lib/xmodule/xmodule/timelimit_module.py (0.0%): Missing line(s) 34,105,111,113
common/lib/xmodule/xmodule/tests/test_conditional.py (100%)
common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py (100%)
common/lib/xmodule/xmodule/x_module.py (88.9%): Missing line(s) 789
lms/djangoapps/instructor/views/legacy.py (100%)
common/djangoapps/xmodule_modifiers.py (86.5%): Missing line(s) 143,144,145,148,149,150,151
lms/djangoapps/courseware/module_render.py (100%)
common/lib/xmodule/xmodule/crowdsource_hinter.py (100%)
common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py (88.9%): Missing line(s) 272
common/lib/xmodule/xmodule/conditional_module.py (100%)
cms/djangoapps/contentstore/views/preview.py (75.0%): Missing line(s) 103
lms/djangoapps/courseware/tests/test_module_render.py (100%)
common/lib/xmodule/xmodule/randomize_module.py (50.0%): Missing line(s) 83
-------------
Total:   109 line(s)
Missing: 15 line(s)
Coverage: 86%
-------------

[sarina]

far gment? f arg ment? hehe this typo can be read in many ways

[sarina]

👍 by me

[nedbat]

This doesn't look right: the if clause sets self.content, the else clause returns? Also, no return value is used from self.render(). Why is there even a self.render() at all? It doesn't seem to be overridden from a base class, and it's only called from a two-line method just above it. I probably missed something.

[cpennington]

No, you're quite right. I think the render method was based on a similar pattern used in other modules (with similar debatable utility). And yes, self.content vs return is clearly wrong.

This is the module I'm working on writing tests of now (because it has exactly 0), so I'm going to leave these in until the tests catch the issues.

[nedbat]

OK, done with the review. None of my comments require re-review, merge when you've addressed whatever needs addressing. 👍

[wedaly]

I'm not okay with including common/lib in the lms/cms test suites within this PR. This is something that has a big impact on how we track and report coverage, and I don't want to make that change without thinking it through carefully and communicating it to everyone who might be affected.

I'll take a closer look at this PR later tonight or early tomorrow to give feedback on the other test infrastructure changes.

[wedaly]

s/deturmined/determined

[wedaly]

Thank you for cleaning up the factories -- I found your implementation much neater, but I think it could use a few more comments to explain what's going on.

To expand on my earlier comment about changing coverage reporting, my main concern is this:

We currently generate several coverage.py HTML reports (lms, cms, xmodule, capa, calc, etc.). If one source file is included in multiple test suites, then it will get reported multiple times, but with different numbers. This is currently how we handle common/djangoapps, and it's pretty confusing.

I'm open to figuring out a better way to collect and report coverage data. One possibility would be to generate a single coverage.py report for edx-platform, so that each source file is reported only once. However, I'd want to get input from all the teams that would be affected (Studio, LMS, and Blades) before making such a big change.

[cpennington]

@wedaly: I reverted the change to the coverages. Regarding that, though. Would it make sense to collect coverage information on all edx-platform source code exercised in either of the LMS or CMS test suites, but then to report on on lms, cms, and the various common directories separately?

(Meaning, if a line in LMS was only covered in a Studio test, it still counts as covered)

[cpennington]

The test that is failing is also failing on master. I'm merging this PR.