CompletionXBlockMixin + tests #368

e-kolpakov · 2017-10-18T04:12:27Z

Description: This PR adds CompletableXBlockMixin mixin to allow XBlocks use Completion API

JIRA tickets: N/A (Part of Completion API work)

Discussions: See Completion API document above.

Dependencies: Dependencies: https://github.com/edx/edx-platform/pull/16234

Merge deadline: Required for the feature with deadline at Nov 27th, 2017 - so no hard deadline, but would be nice to have it merged by Oct 20th, 2017

Author concerns: I'm using hypothesis for property-based testing, but the same can be achieved (likely with more code and covering less cases) with ddt. I find property-based testing a very powerful tool, but it introduces a test dependency (pretty lightweight though) and I haven't explicitly discussed that with anyone. I don't expect to find any significant opposition, but would be nice to vet this decision.

The only non-negligible drawback is that it introduces quite different test style, so to keep "homogenity" it might be desired to stick with ddt. However, as you can see in this PR prop-based testing also supports data-driven test (called table-driven property-checks in that camp), so the two styles can peacefully coexist.

Testing Instructions:

Run tests (should be handled by Travis)

Reviewers

openedx-webhooks · 2017-10-18T04:12:36Z

Thanks for the pull request, @e-kolpakov! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

Create an OSPR issue for this pull request.

jcdyer · 2017-10-18T16:12:09Z

I'll leave the question about hypothesis to the folks at edX. Personally, I like the way it looks. Curious about how many tests it generates and how it affects test suite running time.

Will take a closer look at this soon.

jcdyer · 2017-10-18T20:23:41Z

xblock/VERSION.txt

@@ -1 +1 @@
-1.0.1
+1.1.1


Nit: Should this be 1.1.0?

jcdyer

The code looks good.

I tested this: Ran the test suite, and saw the tests passing. Further manual testing will involve adding it to existing XBlock types, and verifying that they can be completed and uncompleted using the emit_completion method. There are stories to do this.
I read through the code.
~~I checked for accessibility issues~~ N/A
[] Includes documentation (TBD)

jcdyer · 2017-10-18T20:27:08Z

xblock/test/test_mixins.py

+    def test_has_custom_completion_property(self):
+        block = self._make_block()
+        self.assertTrue(block.has_custom_completion)
+        self.assertTrue(getattr(block, 'has_custom_completion', False))


What's the benefit of getting the block both through direct attribute access and through getattr?

jcdyer · 2017-10-18T20:28:12Z

xblock/test/test_mixins.py

+    @example(0.0)
+    def test_emit_completion_emits_event(self, valid_completion_ratio):
+        """
+        Given valid completion ratio


Grammar: Given a valid completion ratio

jcdyer · 2017-10-18T20:28:29Z

xblock/test/test_mixins.py

+    @example(float('-inf'))
+    def test_emit_completion_raises_assertion_error_if_invalid(self, invalid_completion_ratio):
+        """
+        Given invalid completion ratio


Grammar: Given an invalid completion ratio

jcdyer · 2017-10-18T20:29:31Z

xblock/test/test_mixins.py

+            * Less than 0.0
+            * Greater than 1.0
+            * Positive or negative infinity
+            * NaN


Maybe include None, since that's a common value to accidentally pass in.

jcdyer · 2017-10-18T20:34:47Z

xblock/mixins.py

        return hasattr(view, "_supports") and functionality in view._supports  # pylint: disable=protected-access
+
+
+class CompletableXBlockMixin(object):


This and the emit_completion() method should have docstrings. The class docstring should include a brief note regarding why it should be used. (For blocks that need special control over when they are considered complete). The method docstring should explain that emitting a value of 1.0 indicates that a block has been completed, and that emitting a value of 0.0 on a previously completed block indicates that it is no longer considered complete.

jcdyer · 2017-10-18T20:41:18Z

xblock/mixins.py

+    has_custom_completion = True
+    completion_method = 'completable'
+
+    def emit_completion(self, completion_ratio):


I think this method should verify that has_custom_completion is True and completion_method is 'completable'. The value is set here, but I can imagine a scenario where a subclass wants to fall back to default completion calculation. If that subclass sets has_custom_completion = False, it will fall back to one of the default completion implementations, but also emit its own completion events, which will lead to unexpected behavior.

A test that emit_completion exits silently in those cases would be useful.

@jcdyer I don't think it should exit silently - this mixin doesn't have anything useful besides this method - so falling back to default should be performed by not mixing in this class.

If the mixin is not a direct superclass, butfurther up the inheritance chain, that won't be an option. Even though it makes the mixin effectively a noop, that is still the behavior of least surprise and last frustration. The only other options are to cause an error, which will just irritate some poor developer, or to emit the completion anyway, which is exactly the opposite behavior you'd expect when setting has_custom_completion to False.

@jcdyer Let me clarify something - we're discussing behavior of custom completion method when it is called on non-custom-compketion block. To me, it is an outright programming error and should be indicated to programmer assoon as possible.

So to me NoCustomCompletion.emit_completion() is programming error. I realize this mixing can be inherited from some other block, but I can't see a scenario where emit_completion would be rightfully called on non-custom-compketion completion block. If you could describe such scenario I'll gladly revoke this objection.

In any case, if you really strongly believe silently doing nothing is preferred over raising an error and taking into account you'll be doing majority of work on this feature I'll just stick with your preferred way. However I'm not (yet) convinced it is an optimal long-term solution.

jcdyer

👍 On further consideration, and a good night's sleep, I'm happy with this approach.

I tested this: Ran the test suite, and saw the tests passing. Further manual testing will involve adding it to existing XBlock types, and verifying that they can be completed and uncompleted using the emit_completion method. There are stories to do this.
I read through the code.
~~I checked for accessibility issues~~ N/A
Includes documentation

jcdyer · 2017-10-20T14:19:50Z

xblock/mixins.py

+    """
+
+    has_custom_completion = True
+    completion_method = 'completable'


I mentioned on the ticket that I got a request from edx on another ticket to change completion_method to an enum or a set of constants. Would you mind adding that to this PR? Needed values:

completable

aggregator

excluded

@jcdyer I think I've lost track of this request - will do tomorrow.

robrap

Thanks @e-kolpakov.

robrap · 2017-10-24T13:34:41Z

xblock/mixins.py

+        a previously completed block indicates that it is no longer considered complete.
+
+        Arguments:
+            completion_ratio (float): Ratio of completion within [0.0; 1.0] range (inclusive). 0.0 means the block


We agreed to user the term "percent" instead of "ratio". In the model, this is just called "completion". I'm option to "completion", "percent_complete", and "completion_percent", but I'd like to be consistent where and if this appears again (if it hasn't already), so maybe @jcdyer can weigh in.

Note: For consistency, I'd change the doc string to read: "Percent of completion within [0.0; 1.0] range..."

@robrap Please forgive me being pedantic, but "percent" is in range [0.0; 100.0], not [0.0; 1.0]. So just renaming the parameter but keeping allowed range would be extremely misleading. Please consider either:

Keeping this code intact and renaming other occurences to use mathematically correct term.

Change both name and allowed range (if I'm not mistaking it also involves changes down the stream).

Of course I can just rename it to "percent", but it would be a misleading name.

@e-kolpakov I'm totally with you on the distinction, and had originally named it "ratio" on my design doc, but we determined that it's more important to be consistent with edx-platform's general terminology, which is to call floats constrained to [0.0, 1.0] "percent".

We've discussed this at length in other conversations, and I think we should continue as previously decided.

A brief summary:

Ratio isn't precise either, because a ratio could be any range, and it is the fact that it is a percent that requires the range of 0 to 1.

I don't think it is "extremely" misleading. There are places in our code as well as other code bases where a percent is stored or passed around as 0 to 1, even though it will finally become 0-100 when it is displayed with a percent sign.

Worst case scenario, someone coding with this for the first time doesn't read the docs, thinks it is 0 to 100, and quickly finds out they are wrong when 1% is the max percent they are able to reach.

@robrap Is american math or something? :) Ok, I'll do that, but since I have "minority report" I'll add a link to this discussion.

Regarding design doc - I probably haven't reached the part where you guys discuss "ratio" vs. "percent".

Ok, final three things before I wrap up this discussion:

Percent can be any range also (130% ROI, -15% market price drop, etc.).

Ratio is the accurate term here - ratio of 1.0 means "things you're comparing are exactly equal", which in our case corresponds to "you've got maximum score possible".

The worst case scenario you described contributes to bad developer experience. Basically having ratios called percents anywhere edx-platform causes:
3.1. The need to translate name into actual meaning each time someone touches the code operating on grades/progress/etc. - i.e. taxes productivity a little bit.
3.2. And taxes productivity heavier when the scenario you described happens - which is actually not limited to first use, but might also be caused by forgetting the fact, rushing for whatever reason (i.e. hotfix) and maybe some more.
3.3. Long and pointless discussions like this one :)

So, as a member of community I flag this "percent" vs. "ratio" problem as something that reduces my productivity. It's just a tiny bit, but probably you guys (core developers and architects) might consider this as part of technical debt and address when/if there's opportunity to do so.

And sure, I understand effort & risk vs. outcome is somewhere in range "not a chance", but still.

robrap · 2017-10-24T13:39:24Z

xblock/mixins.py

+                )
+            )
+
+        if completion_ratio is None or not 0.0 <= completion_ratio <= 1.0:


Can you drop completion_ratio is None or ?

@robrap It'll cause python 3 tests to fail with TypeError or something like that. Basically python 3 is more picky when it comes to comparing None to anything else.

Got it. Thank you.

robrap · 2017-10-24T13:45:29Z

xblock/test/test_mixins.py

+    @given(strategies.floats(min_value=0.0, max_value=1.0))
+    @example(1.0)
+    @example(0.0)
+    def test_emit_completion_emits_event(self, valid_completion_ratio):


Reminder: when we decide on the proper terms, search the entire PR for "ratio" and replace with the "percent"-like terms. Thanks.

e-kolpakov · 2017-10-25T01:13:19Z

Rebased from 5808436

e-kolpakov · 2017-10-25T02:16:13Z

xblock/test/test_core.py

        """An empty XBlock for testing"""
        pass

+    # FIXME: This test is fragile - fails in python27 if a warning it is expecting is emitted before it is executed


@robrap FYI.

The warnings module has different default filter behavior for different warning categories across different versions of python. Most likely this warning filter is getting set to "once" in this case.

I think this can be worked around by calling warnings.resetwarnings() or warnings.simplefilter("always") in the test setup.

Thanks @e-kolpakov. I'm unclear on the the scope of the fragility. Will it pass 100% of the time in Jenkins based on the current suite, or due to randomization or other issues, can it sometimes fail?

If it can sometimes fail on Jenkins, we have a new flaky test process: https://openedx.atlassian.net/wiki/spaces/TE/pages/161427235/Updated+Flaky+Test+Process
The options are:

Fix the test, or

Delete the test and create a ticket.

If it is just flaky locally depending on how you are running the tests, then the comment may suffice.

@jmbowman: I feel like you may have been dealing with test flakiness. Do you have any thoughts on this? Thanks.

TIL. Thanks @bdero

@jcdyer @bdero No guys, it's not that simple.

@e-kolpakov Have you tried sliding a warnings.resetwarnings() in there before the catch_warnings line? It's totally possible there's a subtle behavior difference between different versions of python (or a bug in the specific release being used), so hopefully playing around with the warning options will yield some good information on the problem.

@bdero Yes, I've tried (and double checked just now) spamming with resetwarnings and filter("always") with different combinations and at different locations - no help.

Unfortunately, I don't have capacity right now to further investigate this issue, so I'd suggest we just wrap it up as is.

@robrap It's not a flaky test - it is a stable test that's easy to break as it depends on the order of tests executed. In short, it only passes now because test_core is run before any other test that could trigger the deprecation warning. If I understand right, nose runs tests alphabetically, so test_core is run very early - only asides and now completion is run earlier. Asides code does not have anything to do with creating XBlocks (where the deprecation warning is set), and completion also doesn't use the deprecated feature - but only after I figured out what's wrong with it - most of the other tests are still using it.

@e-kolpakov @robrap I suspect that sometime reasonably soon we'll want to switch this repository to use pytest instead of nose, and make a concerted effort to fix any dependencies between the tests so they can be run in random order. If the tests are stable until the order changes, I think it's probably ok to leave it as is until we tackle that. pytest has some dedicated functionality for testing warnings which may prove useful when we attempt that.

Thanks @e-kolpakov. You may want to add an additional note regarding @jmbowman's response in the comment if you think it would be helpful to someone coming across this. Or, you could leave a link to this discussion regarding how and when it might get fixed.

robrap

Thanks @e-kolpakov.

robrap · 2017-10-25T13:43:05Z

xblock/test/test_core.py

        """An empty XBlock for testing"""
        pass

+    # FIXME: This test is fragile - fails in python27 if a warning it is expecting is emitted before it is executed


Thanks @e-kolpakov. I'm unclear on the the scope of the fragility. Will it pass 100% of the time in Jenkins based on the current suite, or due to randomization or other issues, can it sometimes fail?

If it can sometimes fail on Jenkins, we have a new flaky test process: https://openedx.atlassian.net/wiki/spaces/TE/pages/161427235/Updated+Flaky+Test+Process
The options are:

Fix the test, or

Delete the test and create a ticket.

If it is just flaky locally depending on how you are running the tests, then the comment may suffice.

@jmbowman: I feel like you may have been dealing with test flakiness. Do you have any thoughts on this? Thanks.

robrap · 2017-10-25T15:08:04Z

xblock/completable.py

+
+    # Completion_percent is somewhat misleading name - as it is actually a ratio. But this is how edx-platform calls it,
+    # so use the same name here for consistency
+    # https://github.com/edx/XBlock/pull/368#discussion_r146560619


@e-kolpakov: First, since I can't respond to the earlier thread, and the earlier thread doesn't include earlier discussions, I'd rather it wasn't linked to here. Instead, I've created a page in Confluence that you can link to and that you can comment on and/or help edit. It was clearly written from the perspective of the decision I already stated, so feel free to copy in your comments.

Of course, percents and ratios can both have any range. I tried to clarify and simplify my thoughts in the document, where it is more about the common understanding of certain phrases. The ranges follow from this understanding.

Lastly, I don't think this comment is as helpful as it could be. If you think it is necessary, I'd say something like: "Note that completion_percent is in the range of [0.0, 1.0], and not [0.0, 100.0] as you might expect." Although, I think the docstring already cover this. Or, if this is to supply a link to the discussion (i.e. the Confluence page), the comment could read something like: "To read more on the debate about using the terms percent vs ratio, see: ...".

Finally, one of the things that some of us agreed upon early was that there was no right and wrong answer here, and we simply need to make some decisions and move on. We did all happen to agree that consistency is better unless there was a really strong reason to be inconsistent.

Thanks again for you work.

robrap · 2017-10-25T15:28:19Z

xblock/completable.py

+            )
+
+        if completion_percent is None or not 0.0 <= completion_percent <= 1.0:
+            raise ValueError("Completion ratio must be in [0.0; 1.0] interval, {} given".format(completion_percent))


Completion percent.

…more informative

e-kolpakov · 2017-10-26T01:58:19Z

@robrap Addressed/commented on your notes, please take another look.

Github UI might be confusing a bit - when one responds to a comment from "Files" tab and clicks "Start my review" / "Add Comment" (default action) it displays the response both in the original thread (with an option to respond to it) and as part of review (without). So, answer to your question about test flakiness is a bit above (here) :)

In such cases I find it slightly more convenient to use "Add Single Comment Only" button - it prevents it from being duplicated in review section.

robrap · 2017-10-26T15:35:06Z

xblock/test/test_core.py

        """An empty XBlock for testing"""
        pass

+    # FIXME: This test is fragile - fails in python27 if a warning it is expecting is emitted before it is executed


Thanks @e-kolpakov. You may want to add an additional note regarding @jmbowman's response in the comment if you think it would be helpful to someone coming across this. Or, you could leave a link to this discussion regarding how and when it might get fixed.

e-kolpakov · 2017-10-29T09:29:33Z

@robrap Thanks - merged. I'm sorry about how it looks on master now - despite squash merging (and fixing those small typos in commit messages) github merged it as is for some reason. Do you think it's worth the trouble squashing them on master (+force push and all that unpleasant things)?

robrap · 2017-10-30T13:20:06Z

I don't think it is worth the trouble, as long as master now has what you imagined it should have. I usually squash and let the tests run on the PR, so I'm not sure how often github doesn't do what you want.

e-kolpakov · 2017-10-30T14:12:11Z

@robrap Actually it doesn't - I wanted it to be a single commit, but it just added all of them without squashing.

robrap · 2017-10-30T15:47:47Z

Right. I just imagined that at least sometimes "squash merge" actually squashes before merging. In any case, it seems like use-at-your-own-risk functionality.

e-kolpakov · 2017-10-30T15:50:02Z

@robrap Normally it squashes, but this time something went wrong. Anyway, I'd prefer to squash them on master to have relatively clean history, so unless you (or someone) have objections I'll do that tomorrow morning (it's almost midnight in my timezone)

robrap · 2017-10-30T16:12:57Z

@e-kolpakov: I don't think you should mess with master until I hear back on our practices. I think the branch is protected and you may not be able to force push to master anyway. It seems our best practices would typically have "squash and merge" disabled, so I'm looking into that. See "Protected Branches" and "Merge Button" sections of this doc: https://openedx.atlassian.net/wiki/spaces/IT/pages/82576458/GitHub+Repository+Admin+Guidelines

robrap · 2017-11-08T17:33:03Z

FYI: @e-kolpakov: Our policy is to not mess with master and we'll just leave this as-is. In the future, you may want to rely on squashing locally to ensure you'll get what you want. Thanks again for the changes.

jcdyer reviewed Oct 18, 2017

View reviewed changes

xblock/VERSION.txt Outdated

@@ -1 +1 @@

1.0.1

1.1.1

Copy link

Contributor

jcdyer Oct 18, 2017

Choose a reason for hiding this comment

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

Nit: Should this be 1.1.0?

jcdyer suggested changes Oct 18, 2017

View reviewed changes

jcdyer approved these changes Oct 19, 2017

View reviewed changes

e-kolpakov requested review from doctoryes and robrap and removed request for doctoryes and robrap October 20, 2017 02:53

jcdyer reviewed Oct 20, 2017

View reviewed changes

robrap reviewed Oct 24, 2017

View reviewed changes

e-kolpakov requested a review from robrap October 25, 2017 01:00

e-kolpakov force-pushed the ekolpakov/completion_mixin branch from 82ae8b6 to 5808436 Compare October 25, 2017 01:04

CompletionXBlockMixin + tests

64eb222

e-kolpakov force-pushed the ekolpakov/completion_mixin branch from 5808436 to 64eb222 Compare October 25, 2017 01:13

e-kolpakov added 2 commits October 25, 2017 10:10

Fixed broken test

d3dcf04

Added (or rather restored) ability to run single test via tox

b2b5d0f

e-kolpakov commented Oct 25, 2017

View reviewed changes

robrap reviewed Oct 25, 2017

View reviewed changes

Fixed one missing ratio ues and made comment about ratio vs. percent …

74ab855

…more informative

robrap approved these changes Oct 26, 2017

View reviewed changes

Added link to discussion in the commetn

734b680

e-kolpakov merged commit b0a6120 into master Oct 29, 2017

e-kolpakov deleted the ekolpakov/completion_mixin branch October 29, 2017 09:18

		return hasattr(view, "_supports") and functionality in view._supports # pylint: disable=protected-access


		class CompletableXBlockMixin(object):

CompletionXBlockMixin + tests #368

CompletionXBlockMixin + tests #368

Uh oh!

Conversation

e-kolpakov commented Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Oct 18, 2017

Uh oh!

jcdyer commented Oct 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcdyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcdyer Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcdyer left a comment

Choose a reason for hiding this comment

Uh oh!

jcdyer Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

e-kolpakov Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

e-kolpakov commented Oct 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdero Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

e-kolpakov commented Oct 18, 2017 •

edited

Loading

jcdyer Oct 18, 2017 •

edited

Loading

jcdyer Oct 20, 2017 •

edited

Loading

e-kolpakov Oct 25, 2017 •

edited

Loading

bdero Oct 25, 2017 •

edited

Loading

e-kolpakov commented Oct 26, 2017 •

edited

Loading