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

Run unit tests GH Actions with tox #139

Merged
merged 8 commits into from
Jun 9, 2022
Merged

Conversation

jonprindiville
Copy link
Member

@jonprindiville jonprindiville commented Apr 13, 2022

Discussion questions:

Which triggers should we use for this action?

It is currently on push and pull_request but maybe that's redundant?

Which Pythons do we actually need to support?

2.7 and 3.5 through 3.9 run OK without too much fuss.

Officially, only 3.7+ are supported today, but I know we've got some use-cases that are behind the times.

We're currently using nose as our test runner, but it doesn't support 3.10 [0,1,2] -- we'll have to deal with that at some point, but maaaaybe not today? Thoughts?

I'm not sure we use many nose-specific features: some marks to indicate which tests are "integration" and also some parametrization in those integration tests (that we won't run on push/PRs anyway).

pytest is nice, but it doesn't support 2.7.

Oh, I guess nose2 [3] is a thing but I don't know much about it other than that the README claims it supports 2.7 and 3.6-3.10

Does anybody care about running tests for Windows/MacOS?

Seems supported by Actions, but IDK if it's simple or necessary 🤷

[0] nose-devs/nose#1122
[1] nose-devs/nose#1118
[2] https://github.com/gadventures/gapipy/runs/6015009047?check_suite_focus=true#step:5:63
[3] https://github.com/nose-devs/nose2

... and remove test-stuff from setup.py because "setup.py test" is
deprecated and we should move away from suggesting/enabling running
tests that way [0]

we'll also pass through an env var for additional nose args so that we
can dodge the "integration" tests by doing the following:

    NOSE_ARGS="-A integration!=1" tox

PLUS! at least temporarily while I'm experimenting, slim down that list
of Python versions (Keep a modern Python 3 + 2.7 for now. Before this is
ready to merge, though, we should find out which other 3.x we should be
testing. The only ones that are officially supported today are 3.7+ but
maybe we have some users that aren't up to date.)

[0] pypa/setuptools#1684
using the `tox-gh-actions` plugin and borrowing heavily from their
example configs at [0]

[0] https://github.com/ymyzk/tox-gh-actions#workflow-configuration
... cause it seems like it failed in Python 3 due to outdated style of
`print` usage [0] which is a bit odd 'cause i ran it successfully
locally

[0] https://github.com/gadventures/gapipy/runs/6014438753?check_suite_focus=true#step:5:50
... cause in `unittest.mock` that method didn't show up until Python 3.6 [0]
but it's easy enough to assert on the length of the list of calls

[0] https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.assert_called_once
@jonprindiville jonprindiville requested a review from marz619 April 13, 2022 22:07
@marz619
Copy link
Contributor

marz619 commented Apr 25, 2022

Which triggers should we use for this action?

I think pull_request is sufficient, but unless we're worried about some kind of limit on running github actions and the relative amount of work that gets done on this codebase, i'm not opposed to having both.

Which Pythons do we actually need to support?

Yea - 2.7 definitely as a number of codebases still use this. Eventually, it would be ideal to split off to having a v2 branch that is dedicated for python2 support and eventually deprecate that once we're no longer using it interally, and release a 3.x.x branch that only supports the currently supported 3.7+ versions of python, but for now, 2.7, 3.5-9 works.

We're currently using nose as our test runner, but it doesn't support 3.10 [0,1,2] -- we'll have to deal with that at some point, but maaaaybe not today? Thoughts?
Would be ideal to move over to pytest as tha is looking like our preferred test framework that most projects are either on or switching to, and at that point maybe get rid of nose entirely, and maybe use tox or some more native tool in GH-Actions to run the tests on the matrix of python versions.

Does anybody care about running tests for Windows/MacOS?

Maybe some users of the library might; however I'm not aware of any usage of this library on Windows Servers so if it's no additional work, sure, but otherwise no. As for MacOS, other than the fact that we largely develop on macs, there are a myriad of tools to run them under a unix-y mode (docker) etc, so nope to having it run natively there.

@jonprindiville
Copy link
Member Author

Which triggers should we use for this action?

I think pull_request is sufficient, but unless we're worried about some kind of limit on running github actions and the relative amount of work that gets done on this codebase, i'm not opposed to having both.

Not worried about limits, no. My understanding from the docs is that Actions is free for public repos.

👍 I'll just leave it on both, then.

Which Pythons do we actually need to support?

Yea - 2.7 definitely as a number of codebases still use this. Eventually, it would be ideal to split off to having a v2 branch that is dedicated for python2 support and eventually deprecate that once we're no longer using it interally, and release a 3.x.x branch that only supports the currently supported 3.7+ versions of python, but for now, 2.7, 3.5-9 works.

IMO if we can get away with managing only a single branch that maintains 2+3 compatibility that may be a bit less mental overhead compared with maintaining two branches but we can just wait and see how things progress to feel out whether that works or not. Probably don't need to figure that out today.

We're currently using nose as our test runner, but it doesn't support 3.10 [0,1,2] -- we'll have to deal with that at some point, but maaaaybe not today? Thoughts?

Would be ideal to move over to pytest as tha is looking like our preferred test framework that most projects are either on or switching to ...

Yeah, I like pytest from my experiences with it. Most recent version only supports Python 3.7+, though, so that might be a factor in our decision.

...and at that point maybe get rid of nose entirely, and maybe use tox or some more native tool in GH-Actions to run the tests on the matrix of python versions.

tox fills a bit of different niche. Whether we want to use pytest or nose or nose2 or plan old unittest or something else as the test runner it may continue to be handy to have tox around to deal with the multiple Pythons. I believe that it's possible to arrange tox configs such that different Pythons get different test-runners, but 🤷 that may or may not be worth the headache.

(I guess this is another don't really need to settle it today kind of conversation, I could open an issue to discuss next test runner moves or we can just wait and see.)

@jonprindiville
Copy link
Member Author

@marz619 @rafikdraoui ping 🔔 (I just got reminded that I left this hanging when some other gapipy Python-support questions were being asked)

@jonprindiville jonprindiville marked this pull request as ready for review June 9, 2022 16:43
Copy link
Contributor

@marz619 marz619 left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽

appreciate the changes to have more explicit @mock.patch v@patch especially since python3 includes it in its stdlib.

@jonprindiville
Copy link
Member Author

Yeah, I suppose I could have just as well done from mock import patch for both Pythons, but 🤷 I like the extra context if nobody else is opposed

@jonprindiville jonprindiville merged commit 1198041 into master Jun 9, 2022
@jonprindiville jonprindiville deleted the tox-in-gh-actions branch June 9, 2022 20:37
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.

2 participants