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: add tox #1262

Merged
merged 2 commits into from
Aug 1, 2023
Merged

ci: add tox #1262

merged 2 commits into from
Aug 1, 2023

Conversation

supakeen
Copy link
Member

tox is a standard testing tool for Python projects, this allows you to test locally with all your installed Python version with the following command:

tox -e py36,py37,py38,py39,py310,py311 -p all

To run the tests in parallel.

@supakeen
Copy link
Member Author

Note, all those tests will currently fail on a mypy error which I plan to attack in a next PR :)

@github-advanced-security
Copy link

You have successfully added a new shellcheck configuration differential-shellcheck. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

@supakeen supakeen force-pushed the add-tox branch 2 times, most recently from fba05bf to ba38d86 Compare March 20, 2023 16:18
@lavocatt
Copy link
Contributor

Maybe, it's good to tackle the mypy error in this PR ? So that it's not blocking someone working on top of this merged commit ?

@lavocatt
Copy link
Contributor

Oh, you already took care of the mypy error.

Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

I'd love this. But let's put it to use in the same PR, in github workflows?

osbuild/api.py Outdated Show resolved Hide resolved
@supakeen supakeen force-pushed the add-tox branch 5 times, most recently from 3b62a30 to dd908ee Compare April 17, 2023 11:58
@supakeen supakeen force-pushed the add-tox branch 8 times, most recently from c5908e4 to 8fa1a57 Compare April 26, 2023 07:40
@supakeen supakeen changed the title tox: add tox ci: add tox Jul 18, 2023
@bcl
Copy link
Contributor

bcl commented Jul 18, 2023

It looks like you are using a new tox feature with -m to select labels. This isn't supported on Fedora 37, so it might be worth mentioning the minimum version of tox that is needed to use it.
It also appears this is not backwards compatible with the older tox version.

@supakeen supakeen force-pushed the add-tox branch 2 times, most recently from 95f8eb0 to de7a855 Compare July 28, 2023 09:36
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

All looks good, but I have one nitpick... You should amend the commit message, since there are no Python-3.6 fixes any more (AFAICT). Otherwise, great job 🥳

`tox` is a standard testing tool for Python projects, this allows you to
test locally with all your installed Python version with the following
command:

`tox -m test -p all`

To run the tests in parallel for all supported Python versions.

To run linters or type analysis:

```
tox -m lint -p all
tox -m type -p all
```

This commit *also* disables the `import-error` warning from `pylint`,
not all Python versions have the system-installed Python libraries
available and they can't be fetched from PyPI.

Some linters have been added and the general order linters run in has
been changed. This allows for quicker test failure when running
`tox -m lint`. As a consequence the `test_pylint` test has been removed
as it's role can now be fulfilled by `tox`.

Other assorted linter fixes due to newer versions:
- use a str.join method (`consider-using-join`)
- fix various (newer) mypy and pylint issues
- comments starting with `#` and no space due to `autopep8`

This also changes our CI to use the new `tox` setup and on top of that
pins the versions of linters used. This might move into separate
requirements.txt files later on to allow for easier updating of those
dependencies.
@supakeen
Copy link
Member Author

@thozza I've removed the reference to py3.6 and py3.7 fixes from the commit message.

thozza
thozza previously approved these changes Jul 28, 2023
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

👨‍🍳 💋

@thozza thozza enabled auto-merge (rebase) July 28, 2023 14:38
@@ -19,17 +19,21 @@ jobs:
- "test.run.test_noop"
- "test.run.test_sources"
- "test.run.test_stages"
- "test.src"
environment:
Copy link
Member

Choose a reason for hiding this comment

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

I just wonder: do we need to test against all of these? We currently support these operating systems:

  • EL 8 (3.6)
  • EL 9 (3.9)
  • Fedora 37 (3.11)
  • Fedora 38 (3.11)
  • Fedora 39 (3.12)

Therefore, I wonder whether we should limit the matrix to just 3.6, 3.9, 3.11, 3.12.

Advantages:

  • lower chance of a random failure
  • less CO2

Copy link
Member Author

@supakeen supakeen Jul 28, 2023

Choose a reason for hiding this comment

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

The versions were selected based on our README (python >= 3.6).

We can disable a few of them but then I'd also like to remove that statement. Isn't it possible to switch interpreters to any of the other versions on EL. I'm reasonably sure I've seen either 3.7 or 3.8 mentioned in bug reports before?

Copy link
Member Author

@supakeen supakeen Jul 28, 2023

Choose a reason for hiding this comment

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

As a side question, do we support operating systems or Python versions with osbuild? Both make sense but I know were also packages for arch and people experiment on Debian and Ubuntu as well.

This limits the list to the Python versions found in current Fedoras,
RHELs, and CentOS Streams.
@supakeen supakeen merged commit 10e7046 into osbuild:main Aug 1, 2023
69 checks passed
@supakeen supakeen deleted the add-tox branch August 1, 2023 13:01
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.

None yet

6 participants