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

Fix error with unit test env caching #71

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sfc-gh-mwyatt
Copy link
Collaborator

We need to ensure we are still installing the latest checked out version of ArcticTraining even when using a cached environment.

@sfc-gh-sbekman
Copy link
Collaborator

sfc-gh-sbekman commented Feb 27, 2025

there is a superior solution to this (besides installing the required packages). It's better to use it instead of dev install - because the latter is not atomic and can be forgotten as attested by this PR.

Instead I always add tests/conftest.py that has:

import sys
from pathlib import Path

# allow having multiple repository checkouts and not needing to remember to rerun
# 'pip install -e .[testing]' when switching between checkouts and running tests.
git_repo_path = str(Path(__file__).resolve().parents[1])
sys.path.insert(1, git_repo_path)

Voila, now pytest will always use the local checkout.

and all kinds of other amazing things can be added to it - those will come later once I start working on testing.

@sfc-gh-mwyatt
Copy link
Collaborator Author

there is a superior solution to this (besides installing the required packages). It's better to use it instead of dev install - because the latter is not atomic and can be forgotten as attested by this PR.

Instead I always add tests/conftest.py that has:

import sys
from pathlib import Path

# allow having multiple repository checkouts and not needing to remember to rerun
# 'pip install -e .[testing]' when switching between checkouts and running tests.
git_repo_path = str(Path(__file__).resolve().parents[1])
sys.path.insert(1, git_repo_path)

Voila, now pytest will always use the local checkout.

and all kinds of other amazing things can be added to it - those will come later once I start working on testing.

Is there a chance we could miss changes that cause error when running pip install? Otherwise, I think this is a nice solution!

@sfc-gh-sbekman
Copy link
Collaborator

Is there a chance we could miss changes that cause error when running pip install?

I'm not sure what you mean? The proposal is for the tests to always use the local checkout - you don't ever need to do pip install -e . before you run tests.

If you need to install some pre-requisite packages to run tests that's a completely different issue which isn't solved by this proposal.

In CI you have to do it anyway for other required packages, so you can just add testing to it.

The proposed change goes beyond CI needs.

@sfc-gh-sbekman
Copy link
Collaborator

Once you added my proposal the original proposal of this PR is no longer needed, because you have already installed all the prerequisites a few lines earlier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants