search

eduNEXT / eox-tenant

Plugin for managing multiple tenants (organizations) within a single Open edX instance.
GNU Affero General Public License v3.0
7 stars 4 forks source link

Tutor integration tests #199

Closed felipemontoya closed 6 days ago

felipemontoya commented 8 months ago

Description

This PR is a proof of concept of how to test the plugin code with different versions of tutor using a real tutor local deployment in github actions

Testing instructions

Additional information

I'm testing with Quince and Palm only because the mounts command started existing then

Checklist for Merge

felipemontoya commented 8 months ago

@Squirrel18 @Alec4r @mariajgrimaldi @andrey-canon I'd like to get your feedback on this. In essence what I'm doing is running tests from this code, a plugin, that run in a tutor local environment created against different versions of tutor.

For this POC I created a simple test that imports the edxapp_wrapper/backends code that was previously unreachable.

I think this test architecture could alter the way we guarantee the support of new versions. For now I was focused on making sure the backends work, but at some point in the future this might even replace the backend pattern.

Note: still working in the tutor-nightly version.

andrey-canon commented 7 months ago

@Squirrel18 @Alec4r @mariajgrimaldi @andrey-canon I'd like to get your feedback on this. In essence what I'm doing is running tests from this code, a plugin, that run in a tutor local environment created against different versions of tutor.

For this POC I created a simple test that imports the edxapp_wrapper/backends code that was previously unreachable.

I think this test architecture could alter the way we guarantee the support of new versions. For now I was focused on making sure the backends work, but at some point in the future this might even replace the backend pattern.

Note: still working in the tutor-nightly version.

We tried to do this a few years ago, and now, thanks to tutor, it's possible :) :) I took a very quick look at the code and looks ok, however this is how implement that but I wonder how this will change our current way of writing plugins, so my questions are:

  1. Will this remove or replace all the edxapp_wrapper folder, so the code will only have explicit imports ?
  2. if the previous answer is yes, what will happen when the import path changes, the plugin will only support current version and probably future versions ?
  3. How will we test in our local environments, the isolate environment approach won't be valid anymore?
  4. Will this be the edunext standard and therefore all the plugins should be migrated to this architecture ?
Squirrel18 commented 7 months ago

In addition to @andrey-canon's comments, last time we spoke about this, we agreed to create a custom Github Action, to implement and reuse this functionality across repositories.

  1. I don't think this work implies that we have to replace the backend_wrapper functionality, as this is mainly a tool to test the plugin against different versions of the platform and not a way to implement a stable API to extend or use functions from the platform, so I think it has different purposes.
  2. In my opinion, I think this should be a standard, however it is important to understand that this is part of an effort to reduce maintenance work in different parts of our Open edX installations and not to make this a standard for eduNEXT repositories.
andrey-canon commented 7 months ago

In addition to @andrey-canon's comments, last time we spoke about this, we agreed to create a custom Github Action, to implement and reuse this functionality across repositories.

  1. I don't think this work implies that we have to replace the backend_wrapper functionality, as this is mainly a tool to test the plugin against different versions of the platform and not a way to implement a stable API to extend or use functions from the platform, so I think it has different purposes.
  2. In my opinion, I think this should be a standard, however it is important to understand that this is part of an effort to reduce maintenance work in different parts of our Open edX installations and not to make this a standard for eduNEXT repositories.

I agree this has theoretically different purposes, I think all of us are aware that if we have, for example, a get_student backend that returns a user from the mysql database, we should be able to exchange that with a get_student backend that returns a user from mongdB, just changing a setting, however are we using that in any plugin ? or we just built flexible plugins that nobody changes, that said, from my point of view, we have two cases, plugins with a backend that includes logic around the openedx dependency, something like this, or plugins whose backends are a simple wrapper to make that any test passes, something like this, I think most of the backends were included just to make that tests pass, so if this allows to test with the real dependency what should we keep this edxapp_wrapper structure when we can explicitly include any dependency ??

felipemontoya commented 7 months ago

Thank you both for your review and comments.

To @andrey-canon's points:

Will this remove or replace all the edxapp_wrapper folder, so the code will only have explicit imports ?

I don't think edxapp_wrapper will be removed as a consequence of this. As I see it, the edxapp_wrapper pattern has 2 main goals:

The isolation is necessary so that we can run quick unit tests. This applies to the CI process, but also to the dev machines. Even if we can run slow integration tests we still need to be able to isolate imports. This would solve your 3rd point, we can still run test locally without having to spin up instances of tutor.

Having the capability to test against several versions might still prove very beneficial here as we can do much more simple edxapp_wrappers and let the tests catch regressions. This has been happening for a while (the simple backends) but we were not properly testing those. Only assuming that stable modules such as user or modulestore would not change much.

if the previous answer is yes, what will happen when the import path changes, the plugin will only support current version and probably future versions ?

This ties nicely with the second goal of the edxapp_wrapper pattern. I think we have made an incomplete and probably incorrect promise tacitly. That a particular version of a plugin supports all the releases it has edxapp_wrapper.backends for. I think we should make a better promise. That a plugin supports all the releases it is tested against. I would also argue that we should only be supporting 2 or 3 releases max for plugins meant for broad use, such as eox-tenant or core. Then we should drop the support and update readmes and docs. Probably delete older backends and make a breaking major bump. I would bet that if you try to run a modern version of eox-tenant agains a "nominally supported" older version of edx-platform, it breaks.

Will this be the edunext standard and therefore all the plugins should be migrated to this architecture ?

I agree with @Squirrel18 in that this should become a standard. But as I wrote before I don't think this is a new arch pattern. We want to write integration tests so that migrating in the future is easier, as we have a safety net to test compatibility instead of relying on manual testing that is slow, costly and might not be as thorough.

Finally @Squirrel18 points that:

we agreed to create a custom Github Action, to implement and reuse this functionality across repositories.

Yes indeed, that happened in a private talk between us. I believe we should make a more public promise about it. We'd like to write this code as a github action so that for plugin maintainers, running this code is as simple as one single step in the workflow.

Thanks again for testing and raising this points.

felipemontoya commented 5 months ago

@MaferMazu published this as an action in: https://github.com/eduNEXT/integration-test-in-tutor

BryanttV commented 6 days ago

https://github.com/eduNEXT/eox-tenant/pull/199#issuecomment-2152789635