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

Github action to run pytest and coverage #191

Conversation

jagerber48
Copy link
Contributor

@jagerber48 jagerber48 commented Feb 5, 2024

Adds a github action that runs pytest and coverage against python versions 3.8 - 3.12.

I think this will require some configuration on the repo side to allow the actions to run.

@jagerber48
Copy link
Contributor Author

On my fork the tests run and pass for all versions jagerber48#1

@andrewgsavage
Copy link
Contributor

Could this be merged? I don't have privaleges to do so.

@jagerber48
Copy link
Contributor Author

I was hoping to get the GitHub actions to actually run before merging but that requires more permissions than I had before. I’ll check if I have more permissions now to make them run.

@wshanks
Copy link
Collaborator

wshanks commented Mar 5, 2024

From the way I interpret https://github.com/orgs/community/discussions/25746, we can't get python-package.yml to run for a PR for testing until some python-package.yml is already in the main branch. An alternative for testing would be pushing to a separate branch in the repo. It seems like GitHub Actions are enabled in the repository settings now. Perhaps we should just merge this and follow up if something doesn't work?

Perhaps we should confirm what matrix to run tests on though. Are we happy just testing on Linux and testing 3.8 through 3.12? Do we want to run tests on Windows and Mac? The full matrix might be overkill but testing at least one Python version on Windows and Mac might be good. Should we remove the appveyor config and integration which tests on Windows and the .travis.yml config which was testing Linux (but seems to be disabled already) here?

@jagerber48
Copy link
Contributor Author

@wshanks ok I see. I was suspicious there was something funny with this being the first time the workflow is showing up.

I'm tempted to create another branch to push to and test it there, but merging and following also sounds reasonable (it worked in my fork).

In the other threads I think we've been converging on testing 3.8-3.12

I don't see any reason not to test on Windows and Mac, I can modify the matrix and see how it goes.

Should we remove the appveyor config and integration which tests on Windows and the .travis.yml config which was testing Linux (but seems to be disabled already) here?

I don't have familiarity with either of those. I see the appveyor check failing. Where does the .travis.yml kick in? If both of them are failing then I'm not opposed to disabling them.

I guess I need to learn (or we need to define) what standard practice is in this repo for merging into master and eventually making releases. e.g. updating the changelog etc.

@jagerber48
Copy link
Contributor Author

Somehow this problem didn't show up on my fork. See jagerber48#1. There the PR adds the new .yaml file that doesn't exist on master, but nonetheless the actions run in the PR.

@jagerber48 jagerber48 self-assigned this Mar 6, 2024
@jagerber48 jagerber48 removed their assignment Mar 6, 2024
@wshanks
Copy link
Collaborator

wshanks commented Mar 6, 2024

An alternative for testing would be pushing to a separate branch in the repo

I think this is why it works on your fork. It can't run across forks until it is on the main branch.

@jagerber48 jagerber48 changed the base branch from master to feature/initial_github_action March 6, 2024 02:48
@jagerber48
Copy link
Contributor Author

If I include windows and mac in the matrix in parallel then there are 12 cases. Feels like kind of a lot, but what are there downsides to having too many test configurations?

For some reason the failing appveyer check went away.

@jagerber48 jagerber48 merged commit c473181 into lmfit:feature/initial_github_action Mar 6, 2024
@jagerber48
Copy link
Contributor Author

jagerber48 commented Mar 6, 2024

Ok, I made a local branch and merged this PR into that one, now I made a new PR from the local branch into local master and the tests run:
#194

Now we should decide exactly how we want the tests to run.

@jagerber48 jagerber48 deleted the feature/github_action branch March 15, 2024 02:23
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.

3 participants