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

tests: fix environment not being replaced soon enough #126

Merged
merged 5 commits into from
Apr 21, 2022

Conversation

onerandomusername
Copy link
Member

@onerandomusername onerandomusername commented Nov 21, 2021

continuation of #120

That solution using a fixture did not patch the environment soon enough. This will patch the environment and save a cache of the original environment, so it can replaced upon the end of a test run.

@onerandomusername onerandomusername added a: CI Continuous Integration and Continuous Deployment a: tests Related to `unittest` tests p: high High Priority skip changelog Skip the need for a changelog entry for a Pull Request labels Nov 21, 2021
@codecov
Copy link

codecov bot commented Nov 21, 2021

Codecov Report

Merging #126 (67d19f5) into main (4b07917) will decrease coverage by 0.38%.
The diff coverage is 47.36%.

❗ Current head 67d19f5 differs from pull request most recent head f503ae6. Consider uploading reports for the commit f503ae6 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #126      +/-   ##
==========================================
- Coverage   70.90%   70.52%   -0.39%     
==========================================
  Files          33       33              
  Lines        1619     1632      +13     
  Branches      199      203       +4     
==========================================
+ Hits         1148     1151       +3     
- Misses        439      447       +8     
- Partials       32       34       +2     
Impacted Files Coverage Δ
tests/modmail/conftest.py 63.88% <47.36%> (-23.07%) ⬇️

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 8ba34ba...f503ae6. Read the comment docs.

@onerandomusername
Copy link
Member Author

After further inspection, this should keep the environment unmodified, only removing all variables prefixed with MODMAIL_ in order to keep the environment more realistic of what would happen on a real environment, and only controlling the program specific environment variables.

@onerandomusername
Copy link
Member Author

However, environment variables are currently not prefixed with MODMAIL_, as that is introduced in #75.

@onerandomusername onerandomusername added the s: deferred Will be done at a later time label Nov 22, 2021
@onerandomusername
Copy link
Member Author

This can be reviewed and merged, but will only have an effect on the testing environment once GH-75 is merged

@onerandomusername
Copy link
Member Author

Merging main to get the updated workflows here.

Will rebase before merge.

@coveralls
Copy link
Collaborator

coveralls commented Nov 27, 2021

Coverage Status

Coverage decreased (-0.05%) to 72.981% when pulling f503ae6 on tests/fix-environment into 8ba34ba on main.

@@ -6,6 +6,11 @@
import pytest


_ORIG_ENVIRON = None

_modmail_env_prefix = "MODMAIL_"
Copy link
Member

Choose a reason for hiding this comment

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

This use modmail.config.ENV_PREFIX once config PR is merged, worth adding a TODO

tests/modmail/conftest.py Outdated Show resolved Hide resolved
@onerandomusername onerandomusername merged commit df5dbb9 into main Apr 21, 2022
@onerandomusername onerandomusername deleted the tests/fix-environment branch April 21, 2022 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: CI Continuous Integration and Continuous Deployment a: tests Related to `unittest` tests p: high High Priority s: deferred Will be done at a later time skip changelog Skip the need for a changelog entry for a Pull Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants