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

add workflow to test pygenesys using gh actions #44

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

yardasol
Copy link
Contributor

This PR adds a workflow to auto-test pygenesis via GitHub Actions.

@yardasol yardasol requested a review from samgdotson August 25, 2022 16:26
@yardasol
Copy link
Contributor Author

I just now saw that this is already set up via circleci. I will close this PR for now, but if we ever want to switch over to GH actions, someone can reopen this PR and try to merge (if it works)

@samgdotson
Copy link
Collaborator

I think we should reopen this, actually. I'm not familiar with testing via github actions vs circle ci. Though, I would like to add a test coverage badge to the readme, which I think needs to be done with github actions. Definitely a worthwhile conversation!

@samgdotson samgdotson reopened this Aug 25, 2022
Copy link
Collaborator

@samgdotson samgdotson left a comment

Choose a reason for hiding this comment

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

I think this is extremely helpful. It won't replace circle ci just yet, but definitely good to include. A couple of notes:

  1. Is temoa installed in the right place? Does it matter? (see comment)
  2. Does temoa need to be installed? It's only necessary if users want to run a temoa simulation -- this may be helpful if I create integration tests using an example model, but right now it serves no purpose.
  3. The .yml file is gobbledygook to me. I would love an explanation offline, sometime.

Comment on lines +6 to +9
paths:
- 'pygenesys/**'
- 'pygenesys/tests/**'
pull_request:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will download Temoa to pygenesys\pygenesys\temoa (per the log file), which is not really where temoa should live in relation to pygenesys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The block you've highlighted doesn't really effect where Temoa is downloaded to. Instead, it indicates that changes to files on the given paths will trigger this workflow.


jobs:
test-pygenesys:
runs-on: ubuntu-latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible to also run-on windows latest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's very possible, would need to slightly readjust some of the syntax, but very easy to do.


jobs:
test-pygenesys:
runs-on: ubuntu-latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
runs-on: ubuntu-latest
runs-on: ${{ matrix.os }}
strategy:
matrix: [ubuntu-latest, windows-latest, macOS-latest]

I think this should do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll want to have a cache for each os.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@yardasol the windows build fails because windows doesn't support the default solver (CBC) for temoa. I don't think our CI should include a temoa build since we only have unit tests. We can introduce a separate check if/when integration tests are introduced. Since pygenesys is separate from temoa, the latter could make breaking changes for their project which would show up as failed checks for this repo -- which isn't our job to handle imo. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's reasonable.

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