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

ci: switch to GitHub Actions #2210

Closed
wants to merge 14 commits into from
40 changes: 0 additions & 40 deletions .github/workflows/Python_tests.yml

This file was deleted.

70 changes: 70 additions & 0 deletions .github/workflows/tests.yml
@@ -0,0 +1,70 @@
# TODO: Line 20, enable python-version: 3.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that Py35 reaches end-of-life in less than two weeks, let's just remove support.
https://devguide.python.org/#status-of-python-branches

# TODO: Line 69, enable pytest --doctest-modules
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems quite important to me. What are the remaining blockers?


name: Tests
cclauss marked this conversation as resolved.
Show resolved Hide resolved

on:
push:
branches:
- master
pull_request:
branches:
- master
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
branches:
- master

I usually run tests on all pull requests on all branches. Is there a reason to avoid doing that?


jobs:
test:
strategy:
fail-fast: false
matrix:
node: [10.x, 12.x, 14.x]
python: [3.6, 3.7, 3.8]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do just one test run on 3.9.0-rc.1 on your own to make sure that we are ready for the Py39 release in one month.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tested it in cc9942e and it passes (only the doctest-modules fails so I reverted it back to a TODO)

os: [macos-latest, ubuntu-latest, windows-latest]
runs-on: ${{ matrix.os }}
steps:
- name: Checkout Repository
uses: actions/checkout@v2
- name: Use Node.js ${{ matrix.node }}
uses: actions/setup-node@v1
with:
node-version: ${{ matrix.node }}
- name: Use Python ${{ matrix.python }}
uses: actions/setup-python@v2
with:
python-version: ${{ matrix.python }}
env:
PYTHON_VERSION: ${{ matrix.python }}
- name: Install Dependencies
run: |
npm install --no-progress
pip install flake8 pytest
- name: Lint node
if: matrix.os == 'ubuntu-latest'
run: |
npm run lint
- name: Lint python
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: Lint python
- name: Lint Python

if: matrix.os == 'ubuntu-latest'
run: |
# stop the build if there are Python syntax errors or undefined names
flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics
# exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide
flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics
- name: Run node tests (Linux and macOS)
if: matrix.os != 'windows-latest'
run: |
npm test
- name: Run python tests (Linux and macOS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: Run python tests (Linux and macOS)
- name: Run Python tests (Linux and macOS)

if: matrix.os != 'windows-latest'
run: |
python -m pytest
- name: Run node tests (Windows)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: Run node tests (Windows)
- name: Run Node tests (Windows)

if: matrix.os == 'windows-latest'
shell: bash
run: |
GYP_MSVS_VERSION=2015 GYP_MSVS_OVERRIDE_PATH="C:\\Dummy" npm test
- name: Run python tests (Windows)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: Run python tests (Windows)
- name: Run Python tests (Windows)

if: matrix.os == 'windows-latest'
shell: bash
run: |
GYP_MSVS_VERSION=2015 GYP_MSVS_OVERRIDE_PATH="C:\\Dummy" python -m pytest
# - name: Run doctests with pytest
# run: pytest --doctest-modules
93 changes: 0 additions & 93 deletions .travis.yml

This file was deleted.

2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -45,6 +45,6 @@
},
"scripts": {
"lint": "standard */*.js test/**/*.js",
"test": "npm run lint && tap --timeout=120 test/test-*"
Copy link
Member

Choose a reason for hiding this comment

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

So, I'd really like this to stay for npm t because it's a good dev workflow. If this is useful to separate out into CI maybe we need a separate "ci" target to use instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added test:ci because personally I find that having testing steps separated helps find root causes for failures easier.

Copy link
Member

Choose a reason for hiding this comment

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

and reverted? was it too complicated?

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 broke windows tests because it added to variables to the environment. Rather go for a smaller changes than go changing things left and right.

"test": "tap --timeout=120 test/test-*"
}
}