Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add test integration #50

Merged
merged 31 commits into from
May 24, 2024
Merged

feat: add test integration #50

merged 31 commits into from
May 24, 2024

Conversation

luisfelipec95
Copy link
Contributor

@luisfelipec95 luisfelipec95 commented May 6, 2024

Description

This PR add integration test file

Testing instructions

  • In a Quince or Palm environment install eox-hooks, I used docker-compose.override.yml
  • Go to bash:
    tutor dev run lms bash
    make test-requirements
    pytest -s --ds=lms.envs.tutor.test /openedx/eox-hooks/eox_hooks/tests/tutor

Description

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

@github-actions github-actions bot added the size/s label May 6, 2024
@Alec4r Alec4r requested review from dcoa and MaferMazu May 6, 2024 20:55
@luisfelipec95 luisfelipec95 force-pushed the lfc/test_integration branch from fa69dba to 4a9ef2b Compare May 6, 2024 20:57
@bra-i-am
Copy link

bra-i-am commented May 7, 2024

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
Copy link
Contributor Author

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

Copy link
Contributor

@MaferMazu MaferMazu left a comment

Choose a reason for hiding this comment

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

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?

@luisfelipec95 luisfelipec95 force-pushed the lfc/test_integration branch from 8944b26 to e0fcf09 Compare May 8, 2024 22:03
@luisfelipec95 luisfelipec95 changed the title add test integration feat: add test integration May 8, 2024
@luisfelipec95
Copy link
Contributor Author

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

Copy link

@bra-i-am bra-i-am left a comment

Choose a reason for hiding this comment

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

Working properly!

image

Copy link
Contributor

@Asespinel Asespinel left a comment

Choose a reason for hiding this comment

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

Screenshot from 2024-05-21 16-54-01
Working as expected.

@MaferMazu
Copy link
Contributor

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
Copy link
Contributor Author

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

@luisfelipec95 luisfelipec95 merged commit bfad685 into master May 24, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants