-
Notifications
You must be signed in to change notification settings - Fork 42
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
chore: pytest hacking #2606
chore: pytest hacking #2606
Conversation
tychoish
commented
Feb 7, 2024
- ran black over the code
- added an env setting
- did a little light refactoring on the dbt tests
- moving some files around
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just one question
@contextlib.contextmanager | ||
def env(key: str, val: str): | ||
prev = os.getenv(key) | ||
|
||
os.environ[key] = val | ||
|
||
try: | ||
yield | ||
finally: | ||
if prev is None: | ||
del os.environ[key] | ||
else: | ||
os.environ[key] = prev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain the need for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
environment variables can either be set, or unset, or set to the empty string, (which is sort of ambiguous.) and the context manager just means that you can have a with
block where the environment variable is set to one thing. and then is restored to its previous state when you exit the block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like pytest is failing but otherwise, assuming no merge conflicts with the latest dbt test PR, this looks good to me.