fix: Solve error for running tests without Zulip API Key

[devkapilbansal]

Description

Fixes #82

Type of Change:

• Code

Code/Quality Assurance Only

• Bug fix (non-breaking change which fixes an issue)
• This change requires a documentation update (software upgrade on readme file)
• New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Checklist:

• My PR follows the style guidelines of this project
• I have performed a self-review of my own code or materials
• I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• Any dependent changes have been merged
• I have written Kotlin Docs whenever is applicable

Code/Quality Assurance Only

• My changes generate no new warnings
• My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been published in downstream modules

[codesankalp]

Just a suggestion why don't we pass the content of the download file as params to Client() instead of passing the file.
I suggest it because we can mock test according to this in the future by making a fake client.
I have attached a screenshot of all params.

[codesankalp]

As in the CI, we need to test the code which uses the zulip_api key.
For reference see this test: #94
How we are going to test this feature @devkapilbansal ?
This test requires download file and the test will surely fail in CI.
Cc: @isabelcosta

[devkapilbansal]

@codesankalp I think it is better to pass whole file in the client. If we are going to mock, then we can mock the whole function value as said by @isabelcosta here. That way we don't need Zulip Api keys in the github workflow

[devkapilbansal]

I wonder if there is a cleaner way to do what I did in this PR

[codesankalp]

I wrote the comments 3 days before this @devkapilbansal, You can ignore the above comments.
But what I want to say is that why don't we import it from settings.py instead of utils.py.

[devkapilbansal]

You want to import client from settings or the whole functions. If talking about whole functions, then zulip_api.py provides not only functions but there examples too and it will create a whole lot of mess if we import from settings.py

[codesankalp]

I suggested because in many projects the client is always defined in the settings.py. But if it will create a mess then leave this point. ☺️

[codesankalp]

Also, @devkapilbansal Is Zulip provides test_api_key for testing purposes?

[isabelcosta]

@devkapilbansal is this still a draft PR?

[devkapilbansal]

@devkapilbansal is this still a draft PR?

Yes @isabelcosta I still don't know what can be the best approach for doing this. Actually getting data by passing fake/mocked key doesn't seems to work

[codesankalp]

@devkapilbansal @isabelcosta
How about using this: https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target ?

If we use this we can access the secrets.

[isabelcosta]

Additionally, @devkapilbansal could you look into https://zulip.readthedocs.io/en/latest/testing/testing-with-django.html#why-is-mocking-useful this has a MagicMock() usage, where i think for Zulip, you can perhaps mock the return value of the call to Zulip. Let me know if you have any doubt. I can also look at this for a bit.

[codesankalp]

@devkapilbansal @isabelcosta
How about using this: https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target ?

If we use this we can access the secrets.

I tried this:
see this: https://github.com/codesankalp/open-source-programs-backend/actions/runs/603804196
The codecov is failing (I will see it tommorrow)

My steps:
I forked my forked repo from my brothers GitHub account and changed the workflow file
workflow file :
https://github.com/codesankalp/open-source-programs-backend/blob/e63f85d6388216b8a59031d39971c07c36892a4a/.github/workflows/tests.yml#L3-L6

And when I created a pull request, the test is able to get zulip_api_key.

Cc: @isabelcosta , @devkapilbansal

[codesankalp]

I think I have figured out the solution to this problem.

Github Actions tests.yml in which I made changes: https://github.com/codesankalp/open-source-programs-backend/blob/develop/.github/workflows/tests.yml
Pull request (all tests passes): codesankalp#6

Test Build on Pull Request: https://github.com/codesankalp/open-source-programs-backend/actions/runs/604945863
Test Build on Push: https://github.com/codesankalp/open-source-programs-backend/actions/runs/604940296

As both builds pass we can take this as the solution to this problem because it will be able to run tests based on zulip_key also.

Cc: @isabelcosta , @devkapilbansal

[isabelcosta]

https://github.com/codesankalp/open-source-programs-backend/blob/develop/.github/workflows/tests.yml

This is very interesting 👍🏾 But I am still wondering 🤔 should we use a real API key everytime we run the tests? I think, to test our zulip functions we should mock the Zulip client responses. We don't want to test Zulip API, we should assume that is covered by that project, we only want to test that our functions return appropriate response when receiving an appropriate response from the API. As an example, while testing get_total_messages_count(), we can mock the response of client.get_messages()

So in this case we want to mock response of this call https://github.com/anitab-org/open-source-programs-backend/blob/develop/osp/utils/zulip_api.py#L40

taking into example what is on official docs: https://zulip.readthedocs.io/en/latest/testing/testing-with-django.html#why-is-mocking-useful

fetch_database = MagicMock(return_value="Mr. Mario Mario")

we can do the same for our client.get_messages(), where we say something like...

get_messages = MagicMock(return_value={
    "messages": [
         item1, item2, item3
     ]
})

internally, the get_messages method will return that, and our function will respond correctly 🤔

[devkapilbansal]

https://github.com/codesankalp/open-source-programs-backend/blob/develop/.github/workflows/tests.yml

This is very interesting 👍🏾 But I am still wondering should we use a real API key everytime we run the tests? I think, to test our zulip functions we should mock the Zulip client responses. We don't want to test Zulip API, we should assume that is covered by that project, we only want to test that our functions return appropriate response when receiving an appropriate response from the API. As an example, while testing get_total_messages_count(), we can mock the response of client.get_messages()

So in this case we want to mock response of this call https://github.com/anitab-org/open-source-programs-backend/blob/develop/osp/utils/zulip_api.py#L40

taking into example what is on official docs: https://zulip.readthedocs.io/en/latest/testing/testing-with-django.html#why-is-mocking-useful

fetch_database = MagicMock(return_value="Mr. Mario Mario")

we can do the same for our client.get_messages(), where we say something like...

get_messages = MagicMock(return_value={
    "messages": [
         item1, item2, item3
     ]
})

internally, the get_messages method will return that, and our function will respond correctly

Looks good. We can try this for testing Zulip APIs. Then we can remove the repository secret for ZULIP_API_KEY. @codesankalp what do you think?

[codesankalp]

Yes, we can do this @isabelcosta @devkapilbansal, and it looks more feasible method than calling the API for testing.

[devkapilbansal]

Also, @devkapilbansal Is Zulip provides test_api_key for testing purposes?

I don't think so 🤔

[codecov-io]

Codecov Report

Merging #102 (efc9207) into develop (f0ed0d8) will decrease coverage by 0.31%.
The diff coverage is 16.66%.

@@             Coverage Diff             @@
##           develop     #102      +/-   ##
===========================================
- Coverage    65.17%   64.85%   -0.32%     
===========================================
  Files           64       64              
  Lines         1025     1030       +5     
===========================================
  Hits           668      668              
- Misses         357      362       +5     

Impacted Files	Coverage Δ
osp/utils/zulip_api.py	23.07% <16.66%> (-5.50%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0ed0d8...efc9207. Read the comment docs.

[codesankalp]

@devkapilbansal I think it will be good if you create all functions mock which uses zulip_api in a new file inside tests directory so that whenever a test will be written in the future the contributor does not need to write the mock again.
I suggested this point as this issue is related to mocking Zulip API.
What's your opinion @devkapilbansal. @isabelcosta.

[devkapilbansal]

@devkapilbansal I think it will be good if you create all functions mock which uses zulip_api in a new file inside tests directory so that whenever a test will be written in the future the contributor does not need to write the mock again.
I suggested this point as this issue is related to mocking Zulip API.
What's your opinion @devkapilbansal. @isabelcosta.

@codesankalp I can but there is already an open issue for writing users test. Therefore, either mocking should be done in that PR or I have to wait for that PR to be merged first using ZULIP_API_KEY.

[isabelcosta]

Cool! Thank you for working on this @devkapilbansal
I hope we can follow up in an issue to test these zulip functions.
Thank you so much for working on this 🙌🏾 @devkapilbansal

[isabelcosta]

I will merge since this is passing CI, but will create a follow-up to make sure we test the Zulip functions.