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

Switch from travis-ci to GitHub workflow. #185

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

Conversation

davidlehn
Copy link
Member

No description provided.

@davidlehn davidlehn force-pushed the add-github-workflow branch 2 times, most recently from cd08e97 to 6423f32 Compare November 3, 2023 22:20
@davidlehn davidlehn force-pushed the add-github-workflow branch from 6423f32 to ca21290 Compare November 3, 2023 22:24
- 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.

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.

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...

Comment on lines +66 to +68
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 }}
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.

@BigBlueHat BigBlueHat added this to the v2.0.4 milestone Feb 5, 2024
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.

Comment on lines +66 to +68
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 }}
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.

@BigBlueHat BigBlueHat modified the milestones: v2.0.4, v2.0.5 Feb 6, 2024
@BigBlueHat
Copy link
Contributor

I'm in favor of getting this merged sooner than later--and it can happen now (vs. as part of a release) since it only affects the master branch and nothing about the running code (or even the tests themselves).

@anatoly-scherbakov we should separate the rdf-canon introduction from #182, so both can move forward at different speeds.

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