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

Use pytest-env to set up API key shims for testing #1382

Closed
1 task
AetherUnbound opened this issue Oct 25, 2022 · 6 comments · Fixed by #3272
Closed
1 task

Use pytest-env to set up API key shims for testing #1382

AetherUnbound opened this issue Oct 25, 2022 · 6 comments · Fixed by #3272
Assignees
Labels
🤖 aspect: dx Concerns developers' experience with the codebase ✨ goal: improvement Improvement to an existing user-facing feature good first issue New-contributor friendly help wanted Open to participation from the community 🟩 priority: low Low priority and doesn't need to be rushed 🧱 stack: catalog Related to the catalog and Airflow DAGs 🐳 tech: docker Involves Docker 🐍 tech: python Involves Python

Comments

@AetherUnbound
Copy link
Collaborator

Problem

Presently we use a copy of the env.template file when testing (via the just dotenv recipe here in the CI/CD workflow)

The env template includes placeholders for the various API keys we use:

https://github.com/WordPress/openverse-catalog/blob/895e907b6f9594b4d43db6a07eae0b3cb4a0e7d0/env.template#L27-L34

Per @krysal's comment (WordPress/openverse-catalog#795 (comment)):

Do you think we should let these variables with no value instead of the "not_set" string? 🤔 That way the scripts that require it will fail with a more clear error, before even trying to make a request, but as it's currently they take the string as the API key.

Description

We should comment these fields out from the env.template file but still keep them present as examples (just like the env.template files for the API). Particularly now that we are no longer retrieving those values at DAG parse time as part of the provider refactor, we no longer need to have these defined in the environment whenever running the project. Leaving them undefined will give us more obvious errors during local DAG testing if we happen to forget to populate a variable.

We do need placeholder values for testing though. That's where we might use the pytest-env plugin to define these with placeholders in the pytest.ini file.

Implementation

  • 🙋 I would be interested in implementing this feature.
@AetherUnbound AetherUnbound added good first issue New-contributor friendly help wanted Open to participation from the community ✨ goal: improvement Improvement to an existing user-facing feature 🐍 tech: python Involves Python 🐳 tech: docker Involves Docker 🟩 priority: low Low priority and doesn't need to be rushed 🤖 aspect: dx Concerns developers' experience with the codebase labels Oct 25, 2022
@Pmeet
Copy link
Contributor

Pmeet commented Nov 2, 2022

Hi @AetherUnbound , I'd like to work on this. But can you elaborate on why we need placeholder values for testing?
I am asking because before that you said you need the obvious errors that will generate when testing.

@AetherUnbound
Copy link
Collaborator Author

Sure @Pmeet, I'll assign it to you! And great call - I think not_set works great as-is. The obvious errors situation is more for the occasions where we're running the DAGs locally ourselves manually rather than during our CI/CD or unit test suite.

@obulat obulat added the 🧱 stack: catalog Related to the catalog and Airflow DAGs label Feb 23, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Openverse Backlog Apr 17, 2023
@obulat obulat transferred this issue from WordPress/openverse-catalog Apr 17, 2023
@ngken0995
Copy link
Collaborator

@AetherUnbound What are the placeholder value in pytest.ini?

@AetherUnbound
Copy link
Collaborator Author

@ngken0995 any value will do! You can just use "placeholder" or "apikey" for testing

@ngken0995
Copy link
Collaborator

@AetherUnbound I have commented all AIRFLOW_VAR_API_KEY from env.template and add them to pytest.ini . Also, install pytest-env and pipfile and pipfile.lock was updated automatically. What is the best way to test the changes?

@AetherUnbound
Copy link
Collaborator Author

I believe if the tests run successfully, that should be sufficient! The tests will fail otherwise if the API keys aren't defined in the env.template.

@obulat obulat moved this from 📋 Backlog to 🏗 In progress in Openverse Backlog Nov 6, 2023
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Openverse Backlog Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 aspect: dx Concerns developers' experience with the codebase ✨ goal: improvement Improvement to an existing user-facing feature good first issue New-contributor friendly help wanted Open to participation from the community 🟩 priority: low Low priority and doesn't need to be rushed 🧱 stack: catalog Related to the catalog and Airflow DAGs 🐳 tech: docker Involves Docker 🐍 tech: python Involves Python
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants