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

incorporate pytest in github workflows #23

Merged
merged 26 commits into from
Jun 24, 2024
Merged

incorporate pytest in github workflows #23

merged 26 commits into from
Jun 24, 2024

Conversation

geoffwoollard
Copy link
Collaborator

@geoffwoollard geoffwoollard commented Jun 20, 2024

#2

@geoffwoollard geoffwoollard self-assigned this Jun 20, 2024
@geoffwoollard geoffwoollard changed the title tests incorporate pytest in github workflows Jun 20, 2024
@geoffwoollard geoffwoollard mentioned this pull request Jun 20, 2024
5 tasks
@geoffwoollard
Copy link
Collaborator Author

geoffwoollard commented Jun 20, 2024

@DavidHerreros please review.

Do you know of a way to cut down on the 3 min for installs? It would be nice to get it down to under 30s, so as to have rapid testing of PRs. Although a developer can also run pytest locally...

@DavidHerreros
Copy link
Collaborator

@DavidHerreros please review.

Do you know of a way to cut down on the 3 min for installs? It would be nice to get it down to under 30s, so as to have rapid testing of PRs. Although a developer can also run pytest locally...

Hi @geoffwoollard

What about this?

The idea would be to cache the virtual environment GitHub actions create during the installation of the pip packages. The first time the tests are run due to a PR, the installation will take the 3 minutes as usual, but in the next executions it wil first check if the environment has been cached an load it if available, reducing almost to zero the installation time.

Also, if any dependency changes in a PR, the whole installation will be redone and cached to consider the new requrements.

I am happy to implement this in the GitHub actions and test it if you think it could be an instersting way to solve the problem 👍

Copy link
Collaborator

@DavidHerreros DavidHerreros left a comment

Choose a reason for hiding this comment

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

I have added omegaconf package to the dev requirements in the project toml file to allow running the test locally as the dependency was missing from the installation.

Apart from that, everything runs great! Before merging we might want to consider the virtul environment cache I commented before to speed up the PR testing.

@geoffwoollard
Copy link
Collaborator Author

@DavidHerreros please review.
Do you know of a way to cut down on the 3 min for installs? It would be nice to get it down to under 30s, so as to have rapid testing of PRs. Although a developer can also run pytest locally...

Hi @geoffwoollard

What about this?

The idea would be to cache the virtual environment GitHub actions create during the installation of the pip packages. The first time the tests are run due to a PR, the installation will take the 3 minutes as usual, but in the next executions it wil first check if the environment has been cached an load it if available, reducing almost to zero the installation time.

Also, if any dependency changes in a PR, the whole installation will be redone and cached to consider the new requrements.

I am happy to implement this in the GitHub actions and test it if you think it could be an instersting way to solve the problem 👍

Please do implement this.

@geoffwoollard geoffwoollard merged commit bd57d28 into main Jun 24, 2024
4 checks passed
@geoffwoollard
Copy link
Collaborator Author

I've merged, but please do submit a PR on this.

DSilva27 pushed a commit that referenced this pull request Aug 6, 2024
incorporate pytest in github workflows
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