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.

Sign up for GitHub

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

Switch from travis-ci to GitHub workflow. #185

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Switch from travis-ci to GitHub workflow. #185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ca21290
Switch from travis-ci to GitHub workflow.
davidlehn Nov 3, 2023
a8db2af
Add flake8 lint workflow.
davidlehn Nov 3, 2023
b9d25d6
Support Python >= 3.8.
davidlehn Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions .github/workflows/main.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
name: Main CI

on: [push]

jobs:
lint:
runs-on: ubuntu-latest
timeout-minutes: 10
strategy:
matrix:
python-version:
- '3.12'
steps:
- uses: actions/checkout@v4
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
- uses: actions/checkout@v4
- uses: actions/checkout@v4
with:
submodules: true

As per https://github.com/actions/checkout

Applies to the below again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to know we can do this, but I agree with @davidlehn that we should get this merged before #182 and can revisit this if/when #182 makes it in.

- name: Use Python ${{ matrix.python-version }}
uses: actions/setup-python@v4
with:
python-version: ${{ matrix.python-version }}
cache: 'pip'
- name: Install testing dependencies
run: |
python -m pip install --upgrade pip
pip install -r requirements-test.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought requrements.txt already contained test dependencies, so could we just add the new packages to that file perhaps?

Copy link
Contributor

Choose a reason for hiding this comment

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

flake8 isn't in requirements.txt, so I think this is till needed for the additional test code below.

Copy link
Member Author

Choose a reason for hiding this comment

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

The intent of requiremenst-test.txt was to avoid those deps being installed other than for testing. No need to install a linter for a regular install. What is best practice for that concept? I saw that somewhere and it seemed a reasonable approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe requirements.txt somehow influences a regular installation of pyld project, like from PyPI. What matters is content of setup.py, which specifies:

    install_requires=[
        'cachetools',
        'frozendict',
        'lxml',
    ],
    extras_require={
        'requests': ['requests'],
        'aiohttp': ['aiohttp'],
        'cachetools': ['cachetools'],
        'frozendict': ['frozendict'],
    }

These are the dependencies. This opens the possibility to use requirements.txt as development dependencies.

As an ideal solution, I would use something like poetry (python-poetry.org) which manages both use cases, but that's a separate question I'd rather not propose doing in this PR.

- name: Lint
# FIXME
continue-on-error: true
run: |
flake8 lib/pyld tests --count --show-source --statistics
env:
LOADER: ${{ matrix.loader }}
test:
runs-on: ubuntu-latest
needs: [lint]
timeout-minutes: 10
strategy:
matrix:
python-version:
- '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.

I 👏 applaud the decision to deprecate currently unsupported Python versions, which are < 3.8.

- '3.9'
- '3.10'
- '3.11'
- '3.12'
- 'pypy3.10'
loader: [requests, aiohttp]
steps:
- uses: actions/checkout@v4
- name: Use Python ${{ matrix.python-version }}
uses: actions/setup-python@v4
with:
python-version: ${{ matrix.python-version }}
cache: 'pip'
- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install -r requirements.txt
- name: Install testing dependencies
run: |
pip install -r requirements-test.txt
- name: Fetch test suites
run: |
git clone --depth 1 https://github.com/w3c/json-ld-api.git _json-ld-api
git clone --depth 1 https://github.com/w3c/json-ld-framing.git _json-ld-framing
git clone --depth 1 https://github.com/json-ld/normalization.git _normalization
Copy link
Contributor

Choose a reason for hiding this comment

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

These three aren't going to be necessary if #182 is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

@davidlehn I would like to do this on top of #182 if possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

  • I believe what I was thinking when this PR was written was to start with testing the code similar to how travis-ci was doing it but with github actions.
  • This will give immediate familiar feedback for testing further changes.
  • Implement git submodules for specs #182 is not addressing the test runner and code changes needed for rdf-canon tests. Every rdf-canon test will be skipped leaving a large hole in coverage if we are to release any versions before the issue is addressed. I don't think changing that test suite before rdf-canon is supported can be justified.
  • It's arguably better to merge this first to get tests flowing. Implement git submodules for specs #182 can be rebased off of it and will then have tests running. The changes to make it use submodules are probably straightforward and would be best done with workflow tests to confirm. (Assuming we can get outside contributor PR tests running properly.)

Copy link
Contributor

Choose a reason for hiding this comment

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

is not addressing the test runner and code changes needed for rdf-canon tests. Every rdf-canon test will be skipped

Should we fix that? I thought I did add that as a dependency and we should be running against that there...

- name: Test with Python=${{ matrix.python-version }} Loader=${{ matrix.loader }}
run: |
python tests/runtests.py ./_json-ld-api/tests -l ${{ matrix.loader }}
python tests/runtests.py ./_json-ld-framing/tests -l ${{ matrix.loader }}
python tests/runtests.py ./_normalization/tests -l ${{ matrix.loader }}
Comment on lines +66 to +68
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
python tests/runtests.py ./_json-ld-api/tests -l ${{ matrix.loader }}
python tests/runtests.py ./_json-ld-framing/tests -l ${{ matrix.loader }}
python tests/runtests.py ./_normalization/tests -l ${{ matrix.loader }}
python tests/runtests.py -l ${{ matrix.loader }}

is going to be sufficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

@anatoly-scherbakov the submodule command will have to be added.

Copy link
Contributor

Choose a reason for hiding this comment

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

@BigBlueHat it seems that actions/checkout has an option to fetch submodules so we can likely avoid running make even.

env:
LOADER: ${{ matrix.loader }}
#coverage:
# needs: [test]
# FIXME
42 changes: 0 additions & 42 deletions .travis.yml
View file
Open in desktop

This file was deleted.

5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
# pyld ChangeLog

## 3.0.0 - 2023-xx-xx

### Changed
- **BREAKING**: Require supported Python version >= 3.8.

## 2.0.3 - 2020-08-06

### Fixed
Expand Down
2 changes: 1 addition & 1 deletion README.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ yet supported.
Requirements
------------

- Python_ (3.6 or later)
- Python_ (3.8 or later)
- Requests_ (optional)
- aiohttp_ (optional, Python 3.5 or later)

Expand Down
1 change: 1 addition & 0 deletions requirements-test.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
flake8
Loading