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

Adopt py.test #3835

Closed
bjlittle opened this issue Sep 9, 2020 · 17 comments
Closed

Adopt py.test #3835

bjlittle opened this issue Sep 9, 2020 · 17 comments

Comments

@bjlittle
Copy link
Member

bjlittle commented Sep 9, 2020

⚙ Feature Request

There are currently a few approaches to running the iris tests as a developer:

  1. using the bespoke iris.tests.runner e.g., python -m iris.tests.runner --default-tests
  2. using the test command from setuptools e.g., python setup.py test
  3. using unittest e.g., python -m unittest discover iris

These are all perfectly functional approaches, for now, to take to run the iris tests. However, there are a few problems with each approach, namely:

  1. iris.tests.runner mechanism is bespoke and heavily relies on nose, which has been in maintenance mode for several years now i.e., it's dead-end technology 💣. Also using this mechanism is causing issues for automatically collating testing coverage statistics.
  2. the use of the test command in setuptools was deprecated in version 41.5.0, (latest 50.3.0) and will be withdrawn in a future version i.e., the future is closer than you think
  3. ummm unittest isn't pytest, not even close 😉

Motivation

It's high time that iris adoped a modern, robust, sustainable, actively supported and scalable approach to testing through pytest and pytest-xdist... before it's too late, and this becomes a much bigger issue than it needs to be i.e., let's agree to plan to avoid this crisis... before it's a crisis.

@bjlittle
Copy link
Member Author

bjlittle commented Sep 9, 2020

@SciTools/iris-devs Thoughts?

@trexfeathers
Copy link
Contributor

ummm unittest isn't pytest, not even close 😉

I would have said yes for this reason alone. Everything else makes me scared to not switch!

@pp-mo
Copy link
Member

pp-mo commented Sep 9, 2020

a few approaches to running the iris tests as a developer:

All that is about the test runner, really, which is about the "total test suite definition", and the needs of CI.

For basic developer needs, I think you missed the really obvious:

  • (4) python -m unittest discover ./lib/iris/tests/...
  • (5) python /path/to/specific/test.py

For sure we should migrate to using pytest as the engine, but the format of our tests has never depended on nose, it is all pure unittest, which pytest also supports quite happily.

So I don't think we need a wholesale rewrite. We can adopt pytest-specific methods as they seem useful,

I do also have a couple of lingering doubts about becoming pytest-specific, though :

  • (1) at one time we likewise considered adopting nose styles, because it looked cool and much neater. In retrospect, that would clearly have been a mistake !
  • (2) I'm still not sure how+whether we can preserve our existing rules for what to test, how to name them, and where to put them (or new equivalent) in the pytest style. For all the tiresome clunkiness of the existing scheme, it does allows us to group + organise things in a useful way, albeit with a bit of extra effort + verbosity.

@bjlittle
Copy link
Member Author

bjlittle commented Sep 9, 2020

@pp-mo Yeah, I wasn't intending to enumerate every possible way to run all the tests, it was just a means to justify my motivation and more importantly get some kinda rough gauge of what everyone else thinks.

  • (4) python -m unittest discover ./lib/iris/tests/...

This is kinda (3) really, but let's not split hairs 😉

  • (5) python /path/to/specific/test.py

Yup, if you wanna run those isolated tests, and all shades of grey of running individual test classes within those, or test class tasks etc

@bjlittle
Copy link
Member Author

bjlittle commented Sep 9, 2020

* (2) I'm still not sure how+whether we can preserve our existing [rules for what to test, how to name them, and where to put them](https://scitools-iris.readthedocs.io/en/latest/developers_guide/tests.html) (or new equivalent) in the pytest style.  For all the tiresome clunkiness of the existing scheme, it does allows us to group + organise things in a useful way, albeit with a bit of extra effort + verbosity.

@pp-mo That a really good point. But at the end of the day I guess we can evolve those iris test name conventions etc as we roll forwards, and make some pragmatic decisions if we want to be more liberal (or not), and factor in the work what needs done and when, and in what order... jeez it's almost like we're gonna plan this 😮

This is a BIG change, and if we're serious about doing it (and I think we should be), how we go about it e.g., blitz it all in a focused sprint, do it piece-meal etc?

@bjlittle
Copy link
Member Author

bjlittle commented Sep 9, 2020

With pytest you can run unittest tests as-is, but that's a bit of a cop out (IMHO) and it kinda paints you into a corner such that you can't fully exploit all the goodness of pytest.

I think that the BIGGEST headache in migration from unittest to pytest for us will be discovering the suitable, common patterns to rewrite the mock base unit tests in pytest 🤯 ... but I did recently take a stab at this for tephi.

I think that it would be wise to get a rough ideal of common approaches we want to adopt for migrating particular testing patterns, otherwise we'll end up with a multitude of different approaches to doing the same thing... and that, I suspect, will be a maintenance headache and increase the bar on developer testing effort

@rcomer
Copy link
Member

rcomer commented Sep 9, 2020

I think we have test examples that would look much cleaner using pytest parametrization, e.g. here two tests that are very similar and the class is subclassed for each of the four arithmetic operations. Or here where each test is looping through the operations. I assume we wouldn’t want to write anything like the latter now, as if your first operation failed the others won’t get tested.

Aside - was there an intention to move/replace all the old modules in the top level tests directory when the unit/integration subdirectories were introduced?

@bjlittle
Copy link
Member Author

bjlittle commented Sep 9, 2020

@rcomer I think that the home of certain tests requires some serious rationalisation and consideration, and for me this would all come under this work. So 👍 on that.

From what I recall (and this is going back some time, perhaps @pp-mo can help clarify) those are legacy tests... and it's a fine line between those tests and integration tests, and system test etc. In fact, the unit tests came later, and those have a little bit more rigour... so IMHO it all needs a bit of a rethink to restructure it in a consistent and coherent way - plus there is most likely a lot of duplicated dead wood to purge/revisit, so we should take the opportunity to do that too.

@bjlittle
Copy link
Member Author

bjlittle commented Sep 9, 2020

I'd also advocate that #3416 is also dealt with as part of this testing re-haul.

@pp-mo
Copy link
Member

pp-mo commented Oct 9, 2020

was there an intention to move/replace all the old modules in the top level tests directory when the unit/integration subdirectories were introduced?

those are legacy tests...

Since @bjlittle asked me directly, yes we intended to retire + replace the "legacy" tests gradually over time.
But re-writing old tests, which are still working fine, has often seemed too much of a thankless task, while busy getting something else done.

@rcomer
Copy link
Member

rcomer commented Jun 11, 2021

A further benefit of pytest is that it discovers tests even if you forget the __init__.py. See #4192.

@cpelley
Copy link

cpelley commented Jun 15, 2021

In ants, we long ago ditched nose in favour of just plane unittests. I can fish out the commit which removed nose from the setup.py if that would help in some way?
However, some of us do choose to use pytest to run the unittests (independently) as it delivers much better output. That is, we decided on the "cop out" as there wasn't an obvious benefit (for us) to make our tests dependent on pytest at the time, but each to his own on running the tests and pytest seems to do a good job of running them.
As an aside, pytest (or unittests for that matter) package itself will not run tests in parallel out of the box unlike nose . Mind you, I'm sure this can be readily achieved with a bit of coding or otherwise I believe there are off the shelf libraries that can provide this capability...

Just a perspective I thought I would share - perhaps there is a good enough reason for writing pytest tests for iris, I don't know. I have always favoured using the standard library myself without strong reason to do otherwise.

@rcomer
Copy link
Member

rcomer commented Jun 15, 2021

To run pytest in parallel, it's just a case of installing pytest-xdist, then simply

pytest -n num_cpus [other args]

I had a quick search for how to parallelise with unittest but didn't come up with anything. Anyone know?

@pp-mo
Copy link
Member

pp-mo commented Jun 15, 2021

how to parallelise with unittest

I believe it basically can't.
That's why we stuck with 'nose' so long, as it was the only way to avoid CI timeouts.

@pp-mo
Copy link
Member

pp-mo commented Oct 26, 2021

Here's another potential benefit
If we commit to PyTest "properly", then we can probably handlings warnings better, including raising all new unexpected ones.
I don't think this is so easy within the existing unittest framework (but I won't say it's not possible).

@rcomer
Copy link
Member

rcomer commented May 10, 2022

I'm delighted to see we are now using pytest to run the tests in CI. Obviously we aren't going to suddenly re-write all our existing tests in pytest style overnight, but is there scope for adding new test modules with pytest tests? I have a branch in progress that I think would cause my future reviewer less pain if I could parametrize some of its tests!

@lbdreyer lbdreyer assigned lbdreyer and unassigned bjlittle Jun 6, 2022
@trexfeathers
Copy link
Contributor

Awesome job @lbdreyer !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants