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

Standardize local and CI testing to use tox #184

Merged
merged 3 commits into from
Sep 22, 2024

Conversation

kurtmckee
Copy link
Contributor

@kurtmckee kurtmckee commented Sep 20, 2024

This PR introduces the following changes:

  • Use tox exclusively to manage test and build environments.

    This change standardizes make and CI invocations of the test suite.

  • Merge Windows testing in with the Linux and macOS test suite.

    This means that Linux and macOS wheels are now getting built and uploaded,
    and also means that coveralls is getting Windows coverage reports.

  • Support building wheels for all supported Python versions.

    This is accomplished using a tox label that expands to all supported Python versions.
    Run tox run -m build or make build to invoke this.

  • Support running the test suite across all Python versions in parallel.

    This is accomplished by running tox run-parallel or simply tox p.
    Cross-environment dependencies, like generating coverage reports after all tests have run,
    are managed in the tox configuration.

@coveralls
Copy link

coveralls commented Sep 20, 2024

Coverage Status

coverage: 59.882% (+30.8%) from 29.101%
when pulling afeff77 on kurtmckee:standardize-testing
into 306fee9 on LudovicRousseau:master.

@kurtmckee kurtmckee force-pushed the standardize-testing branch 3 times, most recently from b087d71 to 7a95190 Compare September 20, 2024 14:42
@LudovicRousseau
Copy link
Owner

Nice.
Can you copy/paste the PR description in the commit message?
The idea is to have the patch itself documented, not just the github PR.

*   Use tox exclusively to manage test and build environments.

    This change standardizes make and CI invocations of the test suite.

*   Merge Windows testing in with the Linux and macOS test suite.

    This means that Linux and macOS wheels are now getting built and uploaded,
    and also means that coveralls is getting Windows coverage reports.

*   Support building wheels for all supported Python versions.

    This is accomplished using a tox label that expands to all supported Python versions.
    Run tox run -m build or make build to invoke this.

*   Support running the test suite across all Python versions in parallel.

    This is accomplished by running tox run-parallel or simply tox p.
    Cross-environment dependencies, like generating coverage reports after all tests have run,
    are managed in the tox configuration.
@kurtmckee
Copy link
Contributor Author

I've amended the commit and force-pushed. 👍

Please let me know if there's anything else that you'd like to see with this change.

@LudovicRousseau
Copy link
Owner

Why do you use choco install swig --version 2.0.12 ...?
Newer version of swig does not work?

Also I don't think the lines:

      - name: "Install build prerequisites (Windows)"
        if: matrix.name == 'Windows'
        run: |
          choco install swig --version 2.0.12 --allow-empty-checksums --yes --limit-output --allow-downgrade
          swig -version

are used.
From the logs I see that "Install build prerequisites (Windows)" is never executed.

Maybe the test should be something like if: matrix.name == 'Windows (x64)' or if: contains(matrix.name, 'Windows')

@LudovicRousseau
Copy link
Owner

tox.ini uses:

pylint --errors-only smartcard

I think it should be:

pylint --errors-only src/smartcard

now that the source code has been moved in src/.

@kurtmckee
Copy link
Contributor Author

Right you are! This worked locally (showing me a number of items that pylint caught) because I had a cached tox environment that had pyscard installed.

I'm fixing this by removing skip_install = True. This will allow pylint to check the module as-installed, rather than checking whatever's in the source tree.

@kurtmckee
Copy link
Contributor Author

Re: swig version

That's a verbatim lift-and-shift of what was in windows.yml. I assumed that that old version was mandatory. I'll remove that downgrade.

Re: Windows setup steps

Yep, I overlooked that during an update. I'll get that fixed.

@kurtmckee
Copy link
Contributor Author

I've pushed an update that uses startsWith() to detect Windows environments, and installs/upgrades swig.

@LudovicRousseau LudovicRousseau merged commit aa16a03 into LudovicRousseau:master Sep 22, 2024
20 checks passed
@kurtmckee kurtmckee deleted the standardize-testing branch September 22, 2024 14:46
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