Move recommender XBlock tests to openedx/tests/external

[nedbat]

I want to make clear that these tests rely on external XBlocks, and that
ideally in time we would move these tests into the XBlock repo.

[nedbat]

@jzoldak @cpennington @pmitros Opinions?
I chose "openedx/tests/external" to make clear that these were tests relying on external code. Jay suggested "openedx/external/tests", but that makes it seem like there will be other external things in there eventually, and I can't think of what those would be. Also, part of the idea of "openedx" is that it should be the new place for anything that would have gone in "common", and since we have "common/tests/acceptance", I figure we'll eventually have "openedx/tests/acceptance".

[pmitros]

I hate the name external. External code to edX? All of our tests rely on libraries external to edx-platform (Django, numpy, etc.). XBlocks in external repositories? That should be all of them -- video player, capa problem, etc., included. Requiring external dependencies? We have a lot of external dependencies. Forums. Insights. Etc. As we move towards a more service-oriented architecture, this set will grow. Many of these are fundamental.

The specific XBlocks which are here are no different than the XBlocks in the edx-platform repo, with the exception that they started life somewhere else. edx-platform has many immature and prototype XBlocks. All the XBlocks should move into their own repos.

What we would want to convey here -- in the long term -- is that these are XBlock/edx-platform integration tests. What we would want to convey here in the short term is that these are XBlock tests.

[nedbat]

@pmitros I meant "external" as in, "outside this repo." We've talked about this a few times; I'm not sure why it bothers you so much.
I want a name that distinguishes between tests that live with the product code they are testing, and tests that are separated from the product code they are testing. "These are tests of code external to this repo" seems like a reasonable distinction to me.

[pmitros]

@nedbat

1. The name is ambiguous. It falls into the same category of bad names like 'class', 'type', 'style.' The name should be self-explanatory. If I do 'ls', I cannot tell the purpose of the directory without looking inside.
2. What goes where is ambiguous. There are zero tests in the platform which do not rely on external code in some way. There is some threshold for externality where it goes in here, but what that threshold is, I do not know. This level of ambiguity will just go up as we factor things like Verticals, Sequentials, and similar into their own XBlocks, and begin to build cross-dependencies with things like Insights.

[nedbat]

I'm not sure where we are going with this. In face-to-face discussions, you seemed sympathetic to the idea that people were uncomfortable with the idea that these tests relied on primary code in another repo, and that that other repo would be a more natural place for these tests to live, IF we could run these tests from there as part of the edx-platform testing.
We all know that we can't run tests from other repos now, so we are putting the tests in edx-platform. We discussed wanting to find a place to put tests like this. I've made a place, and now you seem to be arguing against making the distinction at all.
I'm frustrated, and I don't hear you making a proposal that takes these concerns into account.
I'm proposing to keep these tests here forever. If we get to a world where tests in other repos can be used as part of validating edx-platform, then we'll move these tests into the recommender XBlock repo.

[jzoldak]

Ah, ok. I do like openedx/tests/foo. I was wondering what else would go under openedx/tests, and realize that openedx/tests/integration is likely something that we will want to add later.

But as we have discussed, these really are not integration tests. They are tests that should live in the other repos but cannot at this time because we don't have the ability to test them there.

Maybe we can come to consensus on another word vs. "external" then? A word that signals both "other repo" and "temporary location"?

[pmitros]

@nedbat I'm opposed to the word external, not to the concept of having a place for tests of XBlocks before those tests (and in some cases, tests+XBlocks) can move into those repos. external is incredibly vague, and doesn't make a clear distinction of what goes there.

I'd like something specific, such as xblock-integration, plugin-integration, userspace, or similar. Then, it is clearly where tests for plugins go, rather than ones for all external dependencies. At that point, it becomes clearly the right place for all tests of plugins, including ones that are in repo right now (such as video player).

I'm frustrated, and I don't hear you making a proposal that takes these concerns into account.

I'm tempted to point out the irony here.

[nedbat]

Piotr and I discussed this, and I've used the name openedx/tests/xblock_integration.
Can I get some thumbs-up?

[cpennington]

👍

[jzoldak]

SGTM 👍

[pmitros]

👍

[pmitros]

Wait. I take that back. 👎 until there is a README in the directory explaining what it is.

[nedbat]

I've added a docstring to the __init__.py file explaining what it is for.

[pmitros]

👍