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

More CI - run pytest in github action #95

Merged
merged 3 commits into from
Jul 6, 2020
Merged

More CI - run pytest in github action #95

merged 3 commits into from
Jul 6, 2020

Conversation

jenhagg
Copy link
Collaborator

@jenhagg jenhagg commented Jul 2, 2020

Purpose - automated testing
What it does - Similar setup as powersimdata, but added a couple things:

  • install packages with pipenv
  • clone powersimdata as well so it can be pip installed (this is the prerequisite to running tests)
  • add ability to skip the ci check by putting "skip_ci" in commit messages, useful if you only change a readme or something

Time to review - 10 mins, depending on how much the yaml could be improved. You can see it works by clicking on the green checkmarks, but I'm learning the yaml format as I go so I'm sure it can be optimized somehow.

@jenhagg jenhagg requested review from danielolsen and merrielle July 2, 2020 22:20
@jenhagg jenhagg added this to the Black Lives Matter milestone Jul 2, 2020
Copy link
Collaborator

@merrielle merrielle left a comment

Choose a reason for hiding this comment

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

To be honest I don't have experience with github actions so I can't comment on the finer details, but overall it's pretty easy to tell what's going on. Everything looks good to me.

I'm so stoked we're getting in more CI. :D


jobs:
build:
if: "!contains(github.event.head_commit.message, 'skip_ci')"
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

runs-on: ubuntu-latest
strategy:
matrix:
python-version: [3.6, 3.7, 3.8]
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this line work? Does it test on each of these versions of python?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, that basically creates 3 separate jobs, each with a different value of ${{ matrix.python-version }} which is up to us to use accordingly, in this case by passing it to relevant steps/commands. Some docs if you're interested.

uses: actions/checkout@v2
with:
path: PreREISE
- name: Checkout PowerSimData
Copy link
Collaborator

Choose a reason for hiding this comment

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

if it's possible to include a newline between each step, that would probably make it more readable

@rouille
Copy link
Collaborator

rouille commented Jul 6, 2020

Is it going to clone PoweSimData if it is already installed and possibly overwriting it?

@jenhagg
Copy link
Collaborator Author

jenhagg commented Jul 6, 2020

This only runs on github VMs with a fresh environment each time, so PowerSimData will always need to be cloned.

@rouille
Copy link
Collaborator

rouille commented Jul 6, 2020

This only runs on github VMs with a fresh environment each time, so PowerSimData will always need to be cloned.

Sorry, I forgot the PR was about running tests on GitHub. For some reason I had pipenv in mind.

Copy link
Collaborator

@rouille rouille left a comment

Choose a reason for hiding this comment

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

Looks great. I really like the skip_ci feature.

@jenhagg jenhagg merged commit b0e347d into develop Jul 6, 2020
@jenhagg jenhagg deleted the jon/tests branch July 6, 2020 18:58
@ahurli ahurli mentioned this pull request Mar 16, 2021
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