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.

Sign up for GitHub

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

Jump to bottom

fix: Solve error for running tests without Zulip API Key #102

Merged
merged 1 commit into from
Mar 7, 2021
Merged

fix: Solve error for running tests without Zulip API Key #102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions .github/workflows/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,6 @@ jobs:
python -m pip install --upgrade pip
pip install -r requirements.txt
pip install coverage==5.4

- name: Creating Zulip API key
run: echo -e "${{ secrets.ZULIP_API_KEY }}" >> download

- name: Migrating Test Database
run: python manage.py migrate
Expand Down
9 changes: 8 additions & 1 deletion osp/utils/zulip_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@

# By default the API key file you download is named as 'download' by Zulip. You can place
# this file one directory previous to the cuurent directory your file is in
client = Client(config_file="download")


def get_client():
Copy link
Member

@codesankalp codesankalp Feb 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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. ☺️

return Client(config_file="download")


def get_zulip_user(zulip_id: int) -> dict:
Expand All @@ -15,6 +18,7 @@ def get_zulip_user(zulip_id: int) -> dict:
{"avatar_url": "...","bot_type": null,... "profile_data":...}
"""

client = get_client()
result = client.get_user_by_id(zulip_id)
print(result)
return result["user"]
Expand All @@ -31,6 +35,7 @@ def get_total_messages_count(zulip_id: int) -> int:
10
"""

client = get_client()
request = {
"anchor": "newest",
"num_before": 5000,
Expand All @@ -52,6 +57,7 @@ def get_newest_message(zulip_id: int) -> dict:
{"avatar_url": ...,"bot_type": null, ..., "profile_data":...}
"""

client = get_client()
request = {
"anchor": "newest",
"num_before": 1,
Expand All @@ -73,6 +79,7 @@ def get_stream_messages_count(stream: str, zulip_id: int) -> int:
10
"""

client = get_client()
request = {
"anchor": "newest",
"num_before": 1,
Expand Down