-
Notifications
You must be signed in to change notification settings - Fork 229
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
Don't run tests for non-code changes #898
Changes from 16 commits
971ee8a
6ef324a
a8b5320
e888b99
d1e18a3
db47e39
b1ccbd7
7a70964
5c05104
49704d8
b192f8b
0539fd1
d81c7c5
629e4de
b009772
67d01ca
7980816
28cf07b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
name: Test | ||
|
||
on: | ||
pull_request: | ||
branches: [main] | ||
paths: '**.py' | ||
push: | ||
branches: [main] | ||
paths: '**.py' | ||
|
||
env: | ||
# The lower bound from the pyproject.toml file | ||
OLDEST_PYMC_VERSION: "$(grep -E 'pymc *>' pyproject.toml | sed -n 's/.*>=\\([0-9]*\\.[0-9]*\\.[0-9]*\\).*/\\1/p')" | ||
|
||
jobs: | ||
test: | ||
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
config: [ {python-version: "3.10", oldest-pymc: false}, {python-version: "3.12", oldest-pymc: true}] | ||
steps: | ||
- uses: actions/checkout@v3 | ||
- name: Set up Python | ||
uses: actions/setup-python@v3 | ||
with: | ||
python-version: ${{ matrix.config.python-version }} | ||
- name: Install oldest version of PyMC | ||
if: ${{ matrix.config.oldest-pymc }} | ||
run: pip install pymc==${{ env.OLDEST_PYMC_VERSION }} | ||
- name: Run tests | ||
run: | | ||
pip install -e .[test] | ||
pytest --cov-report=xml --no-cov-on-fail --durations=50 | ||
- name: Check oldest version of PyMC | ||
if: ${{ matrix.config.oldest-pymc }} | ||
run: python -c "import pymc; assert pymc.__version__ == '${{ env.OLDEST_PYMC_VERSION }}'" | ||
- name: Upload coverage to Codecov | ||
uses: codecov/codecov-action@v3 | ||
with: | ||
token: ${{ secrets.CODECOV_TOKEN }} # use token for more robust uploads | ||
name: ${{ matrix.config.python-version }} | ||
fail_ci_if_error: false | ||
|
||
test_slow: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v3 | ||
- name: Set up Python | ||
uses: actions/setup-python@v3 | ||
with: | ||
python-version: "3.10" | ||
- name: Run tests | ||
run: | | ||
pip install -e .[test] | ||
pytest --only-slow --cov-report=xml --no-cov-on-fail --durations=50 | ||
- name: Upload coverage to Codecov | ||
uses: codecov/codecov-action@v3 | ||
with: | ||
token: ${{ secrets.CODECOV_TOKEN }} # use token for more robust uploads | ||
name: "test_slow" | ||
fail_ci_if_error: false | ||
|
||
all: | ||
if: ${{ always() }} | ||
runs-on: ubuntu-latest | ||
name: All checks | ||
needs: [ test, test_slow ] | ||
steps: | ||
- name: Confirm checks passed | ||
if: ${{ (needs.test.result != 'success' || needs.test_slow.result != 'success') }} | ||
run: exit 1 | ||
Comment on lines
+63
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this really required? @juanitorduz This seems to be hanging in the github ui as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be honest, I did not write this part of the CI. I agree it is not bringing much (I hope we are not missing something) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can keep and remove the if: ${{ always() }} CC: @ricardoV94 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The default behavior is exiting if either test or test_slow fail. Right now we have that functionality turned off with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Lets remove that to make sure they fail the workflow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason for delay failing is all jobs are run instead of stopping when the first one fails. In my experience this ultimately saves on CI runs because we have a list of all failing tests that can be checked locally (since nobody runs the whole suite locally by default). Also the first to fail can be the least informative There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. Do you have a recommendation on this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Nope, didn't check the other changes to know what's the context. Just wanted to shine light on this bit |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
name: Test Notebook | ||
|
||
on: | ||
pull_request: | ||
branches: [main] | ||
paths: | ||
- '**.py' | ||
- '**.ipynb' | ||
push: | ||
branches: [main] | ||
paths: | ||
- '**.py' | ||
- '**.ipynb' | ||
|
||
env: | ||
# The lower bound from the pyproject.toml file | ||
OLDEST_PYMC_VERSION: "$(grep -E 'pymc *>' pyproject.toml | sed -n 's/.*>=\\([0-9]*\\.[0-9]*\\.[0-9]*\\).*/\\1/p')" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dont think this is needed in this file There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're correct it's not. |
||
|
||
jobs: | ||
example_notebooks: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v3 | ||
- name: Set up Python | ||
uses: actions/setup-python@v3 | ||
with: | ||
python-version: "3.12" | ||
- name: Install dependencies | ||
run: | | ||
sudo apt-get install graphviz | ||
pip install -e .[docs] | ||
pip install -e .[test] | ||
- name: Run notebooks | ||
run: make run_notebooks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we add
*.ipynb
to see if a change in a notebook that will break theexample_notebooks
job will trigger the CI and fail as expected?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about putting that into a separate action? The tests will only need to be run with package changes. The notebooks with package or notebooks themselve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good idea to separate them out. We would want to trigger the
example_noteboooks
on any change to an.ipynb
or.py
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets give the separation a try