[BD-21] Make use of edx_toggles newest API for testing by regisb · Pull Request #118 · openedx/completion

regisb · 2020-10-23T14:00:48Z

Description: Make use of edx_toggles API for overriding waffle switches in tests

JIRA: https://openedx.atlassian.net/wiki/spaces/COMM/pages/1596358943

Dependencies: openedx/edx-toggles#75 ✔️ merged

Reviewers:

@robrap

Merge checklist:

All reviewers approved
@robrap to confirm deprecated_waffle_utils custom attribute data exists before landing this
CI build is green
Version bumped
Changelog record added
Documentation updated (not only docstrings)
Commits are squashed

Post merge:

Create a tag
Check new version is pushed to PyPi after tag-triggered build is
finished.
Delete working branch (if not needed anymore)

openedx-webhooks · 2020-10-23T14:00:54Z

Thanks for the pull request, @regisb! I've created BLENDED-642 to keep track of it in Jira. More details are on the BD-21 project page.

When this pull request is ready, tag your edX technical lead.

regisb · 2020-10-23T14:32:40Z

This is ready for review @robrap.

robrap · 2020-10-24T01:00:39Z

            override (bool): True if tracking should be enabled.
        """
-        _waffle_overrider = waffle.waffle().override(waffle.ENABLE_COMPLETION_TRACKING, override)
+        _waffle_overrider = override_waffle_switch(waffle.waffle_switch(waffle.ENABLE_COMPLETION_TRACKING), override)


If this is working, can we just leave this as-is until we have the final version of all of this code without namespacing, so we don't have to test and retest this with every revision? We'll just add it once the dust settles.

This is unfortunately not working, as we remove the .override methods from the waffle namespace classes in edx-platform. edx-platform tests themselves depend on edx-completion's test_utils, so tests will not pass until we merge this.

I presume you mean in this PR: https://github.com/edx/edx-platform/pull/25417/files?

In the edx-platform, I'd rather deprecate and then remove. I usually add something like set_custom_attribute('deprecated_THING_TO_BE_DEPRECATED', True) to the deprecated methods so I can track that there really is no usage before removal.

We can still land this, but it might be better to land it after, rather than before, so I can see that the deprecated_X custom attribute was firing and finally stops. The main point is to see if anything else is using it that we didn't notice before removal.

Yes, this is linked to https://github.com/edx/edx-platform/pull/25417.

The override functions are test methods, so my thinking was that it would be overkill to go through the deprecation process for them. But sure, I can do that.

We can still land this, but it might be better to land it after, rather than before, so I can see that the deprecated_X custom attribute was firing and finally stops. The main point is to see if anything else is using it that we didn't notice before removal.

This needs to be merged before https://github.com/edx/edx-platform/pull/25417, or unit tests will fail in edx-platform -- unless we go through a deprecation process.

regisb · 2020-10-29T08:59:27Z

@robrap I updated this PR -- and did not squash my changes this time :)

robrap

In terms of timing, assuming we add a custom attribute for monitoring deprecation of the edx-platform WaffleSwitch and WaffleSwitchNamespace before removing them, I'd like to land this after the edx-platform deprecation, and before the removal.

regisb · 2020-10-29T21:14:44Z

I'd like to land this after the edx-platform deprecation, and before the removal.

Sounds good. I'll add back the override method to https://github.com/edx/edx-platform/pull/25417 so that we can merge this completion PR after.

robrap

I plan on delaying merge until I can see the monitoring custom attribute from the edx-platform PR. Please ensure this is squashed as you wish.

robrap · 2020-11-03T00:18:34Z

+# .. toggle_description: Indicates whether or not to track completion of individual blocks. Keeping this disabled
+#   will prevent creation of BlockCompletion objects in the database, as well as preventing completion-related
+#   network access by certain xblocks.
+# .. toggle_use_cases: open_edx


@regisb: Do you know that this is meant to be a permanent flag, or was this just a guess?

My bet is that this is meant to be a platform-togglable flag, because I was bit by this in Tutor: https://discuss.overhang.io/t/course-not-starting/962/12 I assumed that the fact that the flag was not enabled by default was a feature, and not a bug. Also, this flag has been there for a very long time, so even if the flag was meant to be temporary, it's now permanent.

Sounds good. I'm not clear on why it is an option and why it is off by default. I asked the team and we'll see.
Either way, we can merge and it can always be updated later.

regisb · 2020-11-03T08:40:44Z

I plan on delaying merge until I can see the monitoring custom attribute from the edx-platform PR. Please ensure this is squashed as you wish.

Yes, this makes sense. I rebased my changes so this should be ready to merge once you see the deprecation warnings in edx-platform.

openedx-webhooks added blended PR is managed through 2U's blended developmnt program needs triage labels Oct 23, 2020

regisb force-pushed the regisb/waffle-override branch from 89f6426 to a438a44 Compare October 23, 2020 14:32

regisb force-pushed the regisb/waffle-override branch from a438a44 to 92e5cc3 Compare October 23, 2020 14:40

robrap reviewed Oct 24, 2020

View reviewed changes

regisb mentioned this pull request Oct 26, 2020

[BD-21] Switch to edx-toggles classes and deprecate waffle override methods openedx/openedx-platform#25417

Merged

regisb force-pushed the regisb/waffle-override branch from 92e5cc3 to 506925e Compare October 26, 2020 19:37

robrap reviewed Oct 29, 2020

View reviewed changes

Comment thread completion/waffle.py Outdated

regisb force-pushed the regisb/waffle-override branch from 3db74f1 to 67f276a Compare October 29, 2020 09:15

robrap reviewed Oct 29, 2020

View reviewed changes

Comment thread completion/test_utils.py

robrap approved these changes Nov 3, 2020

View reviewed changes

[BD-21] Make use of edx_toggles newest API for testing

0df071a

regisb force-pushed the regisb/waffle-override branch from 67f276a to 0df071a Compare November 3, 2020 08:38

robrap merged commit e1db6a1 into openedx:master Nov 4, 2020

openedx-webhooks added merged and removed needs triage labels Nov 4, 2020

regisb deleted the regisb/waffle-override branch November 5, 2020 08:38

Uh oh!

Conversation

regisb commented Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Oct 23, 2020

Uh oh!

regisb commented Oct 23, 2020

Uh oh!

robrap Oct 24, 2020

Choose a reason for hiding this comment

Uh oh!

regisb Oct 26, 2020

Choose a reason for hiding this comment

Uh oh!

robrap Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

regisb Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

regisb commented Oct 29, 2020

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

regisb commented Oct 29, 2020

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

robrap Nov 3, 2020

Choose a reason for hiding this comment

Uh oh!

regisb Nov 3, 2020

Choose a reason for hiding this comment

Uh oh!

robrap Nov 3, 2020

Choose a reason for hiding this comment

Uh oh!

regisb commented Nov 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

regisb commented Oct 23, 2020 •

edited

Loading