search

eduNEXT / eox-hooks

GNU Affero General Public License v3.0
3 stars 0 forks source link

feat: add test integration #50

Closed luisfelipec95 closed 5 months ago

luisfelipec95 commented 6 months ago

Description

This PR add integration test file

Testing instructions

Description

https://edunext.atlassian.net/browse/DS-907

bra-i-am commented 6 months ago

hi @luisfelipec95, I've been taking a look at this PR and it raised a doubt: the tests passed correctly, but it shows me an error/warning/exception idk... with a trigger event that you use in your TestPostToWebhook class:

image

Do you happen to know if this warning comes from before, should I run any other step or does it need a fix?

luisfelipec95 commented 6 months ago

hi @luisfelipec95, I've been taking a look at this PR and it raised a doubt: the tests passed correctly, but it shows me an error/warning/exception idk... with a trigger event that you use in your TestPostToWebhook class:

image

Do you happen to know if this warning comes from before, should I run any other step or does it need a fix?

Hi @bra-i-am , thanks for the feedback, this warning comes from before on eox-hooks/eox_hooks/actions_handler.py, a separate solution is needed

luisfelipec95 commented 6 months ago

Thanks for this PR, @luisfelipec95. I have some feedback, and I hope it helps you.

I think we won't need the TestPostToWebhook because we already have tests for that here: https://github.com/eduNEXT/eox-hooks/blob/master/eox_hooks/tests/test_actions.py#L18.

Another thing we won't need is the import of the test modules because, as you use mocks for tests, those test modules too and don't have integration with openedx (the test modules shouldn't import from openedx). But you can double-check and remove those imports if so.

And I think test_runs_code is not necessary either. What do you think?

These tests have a little problem because they show a false positive. I used the instructions here: https://docs.google.com/document/d/1l-MPWMo_vwCRK9A8qupv5kAcoQ799Sj5uoq6e7p3hck/edit?usp=sharing to test Django Apps. The test works well, but it should fail because I didn't install Tutor or run it in an environment that contains openedx.

The false positive is happening because when we import the backends, those backends don't import the openedx modules directly; they import them inside functions. To solve this, I recommend extracting the imports of the openedx modules from the functions. With these changes, things such as course_modes_j should fail, and we should remove it from the test because it is explicit that that module uses the openedx module for Juniper. If we have course_modes_l, it is because the backend J failed.

On the other hand, what does POC mean in line 13? Is it proof of concept or something like that? If so, can we remove that, because this is not? By the way, can you help me drop the commits you left in the master?

Thank you very much for the feedback, corrections were made

MaferMazu commented 5 months ago

This looks good to me, but as I said in our meeting, it would be great if you could add an API test if you think it could enter the scope; if not, we can go with this because it works.

luisfelipec95 commented 5 months ago

This looks good to me, but as I said in our meeting, it would be great if you could add an API test if you think it could enter the scope; if not, we can go with this because it works.

Yes, I think that test is important, it will be done with the integration of GitHub Actions with all the plugins. Thanks