Replace nose with pytest #34778

millerdev · 2024-06-14T19:38:37Z

https://dimagi.atlassian.net/browse/SAAS-14431

🐡 Review by commit.

Safety Assurance

Safety story

Changes tests and documentation with a couple minor exceptions:

corehq/util/decorators.py - fix Django settings access on import
corehq/util/metrics/const.py - related to previous

Automated test coverage

Absolutely.

Rollback instructions

This PR can be reverted after deploy with no further considerations

Required by --strict-markers

test_user_types tests were not being run because the "yield" tests were masked by @patch_user_data_db_layer, which converted the generator to a normal function, which was then "run" as a test but didn't actually test anything.

A few tests are also now found that nose was missing.

- Disable cacheprovider plugin since it is not possible to re-run failed tests in that context. - Add option to show local variables in test failure tracebacks.

Tried using capsys, but couldn't make it work.

Also a few other non-test files.

kaapstorm

(Partial review with Matt. To be continued.)

kaapstorm · 2024-10-25T14:46:19Z

requirements/test-requirements.in

+# pytest is pinned to a git commit until 8.4 is released.
+# `minversion = 8.4` should also be set in .pytest.ini at that time.
+pytest @ git+https://github.com/pytest-dev/pytest.git@85760bff2776989b365167c7aeb35c86308ab76b


What feature are we using that will be in 8.4?

pytest-dev/pytest#12536

kaapstorm · 2024-10-25T14:52:20Z

manage.py

@@ -40,10 +43,9 @@ def main():
    run_patches()

    from corehq.warnings import configure_warnings
-    configure_warnings(is_testing(sys.argv))
+    configure_warnings()


Should configure_warnings() take the CCHQ_TESTING env var into account?

Fair question. Should a management command (that is not running tests) fail because of warnings if CCHQ_TESTING is set? For example, if you are running CCHQ_TESTING=1 ./manage.py migrate to migrate your test database, should it crash if warnings are emitted? I think that might be more annoying than helpful, but I'm happy to hear arguments in favor.

We reckoned that CCHQ_TESTING=1 ./manage.py dbshell would useful for inspecting the test database.

Are you saying you agree that that command should not crash on a warning? That's my position.

kaapstorm · 2024-10-25T14:53:50Z

corehq/tests/pytest_plugins/patches.py

+
+def patch_assertItemsEqual():
+    import unittest
+    unittest.TestCase.assertItemsEqual = unittest.TestCase.assertCountEqual


(Why do we currently do this? Do we not test the values of the items?)

assertItemsEqual was removed, possibly as long ago as when we upgraded to Python 3, but we still have code that uses it. This was easier than changing all the places that use it. It's not new with this PR, it was moved from here.

Interesting. Yeah, it looks like Python renamed assertItemsEqual to assertCountEqual in Python 3.2.

kaapstorm · 2024-10-25T15:08:09Z

corehq/tests/test_locks.py

@@ -51,7 +52,7 @@ def simulate_reentrant_lock():
 @timelimit(0.1)
 def test_unreleased_lock():
    msg = "unreleased dict_values([ReentrantTestLock(name='unreleased', level=1)])"
-    with assert_raises(AssertionError, msg=msg):
+    with assert_raises(AssertionError, msg=re.compile("^" + re.escape(msg))):


Is there a reason why msg can now be longer, or does this fix something that is currently wrong?

This became necessary because pytest changes the assertion error message, I think as part of its assertion introspection logic:

> assert not locks, f"unreleased {locks.values()}" E AssertionError: unreleased dict_values([ReentrantTestLock(name='unreleased', level=1)]) E assert not {'unreleased': ReentrantTestLock(name='unreleased', level=1)} corehq/tests/locks.py:58: AssertionError _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ ... snip > self.gen.throw(typ, value, traceback) E AssertionError: not equal E "unreleased dict_values([ReentrantTestLock(name='unreleased', level=1)])\nassert not {'unreleased': ReentrantTestLock(name='unreleased', level=1)}" E "unreleased dict_values([ReentrantTestLock(name='unreleased', level=1)])" ../../.pyenv/versions/3.9.17/lib/python3.9/contextlib.py:137: AssertionError

biyeun · 2024-10-31T20:36:57Z

corehq/tests/noseplugins/elasticsnitch.py

-from corehq.apps.es.client import manager
-
-
-class ElasticSnitchPlugin(Plugin):


@millerdev curious...but is something like this still useful/needed? Do we have a backlog to track re-implementing some of these plugins with pytest? I know the commit message says out of scope, but this one in particular seems like it was added fairly recently with the ES migration work (2023) by Joel.

My thought was we can reimplement this if/when we need it. I doubt anyone has used it since Joel finished his ES work, but I'd be happy to port it to pytest if someone wants it. Do you use it?

gherceg

Paired with Daniel to review this. Left a couple of non-blocking questions. Great job @millerdev 🎆

gherceg · 2024-11-05T18:46:42Z

testsettings.py

@@ -58,40 +58,8 @@
    'testapps.test_pillowtop',
 ) + tuple(INSTALLED_APPS)  # noqa: F405

-TEST_RUNNER = 'django_nose.BasicNoseRunner'
-NOSE_ARGS = [
-    #'--no-migrations' # trim ~120s from test run with db tests


Should look to see if test run time increases by ~120s after switching to pytest.

I think the answer is possibly "yes." The average build time does appear to have increased from 14 minutes to 16 minutes soon after Nov 11 when this PR was merged.

gherceg · 2024-11-05T20:40:09Z

corehq/tests/test_reusedb.py

+    with pytest.raises(AttributeError, match="Mock object has no attribute 'info'"):
+        CouchModel.get_db().info


I haven't looked closely, but I assume there is no way to get this to raise a RuntimeError similar to the other db access attempts?

Ha in hindsight I think this was a silly comment for me to make. I was imagining a way to raise a RuntimeError in how CouchModel was invoked, not necessarily just having the mock translate an AttributeError into a RuntimeError for the sake of it. I'll leave on a comment on the PR as well.

gherceg · 2024-11-05T20:45:47Z

corehq/tests/pytest_plugins/reusedb.py

-    config.reuse_db = reusedb = config.getoption("--reusedb")
-    config.skip_setup_for_reuse_db = reusedb and reusedb != "reset"
+    config.reuse_db = reusedb = config.getoption("--reusedb") or config.getvalue("reuse_db")
+    config.skip_setup_for_reuse_db = reusedb and (reusedb != "reset" or config.getvalue("create_db"))


I'd expect that if create_db were true, we wouldn't skip setup?

The docs for --create-db say:

Re-create the database, even if it exists. This option can be used to override --reuse-db.

It will not skip setup if create_db is true unless reusedb is not false.

Another way to say that is create_db has no effect unless reusedb is true. Databases are always (re)created unless reusedb is true.

millerdev added the product/invisible Change has no end-user visible impact label Jun 14, 2024

dimagimon added dependencies Pull requests that update a dependency file Risk: High Change affects files that have been flagged as high risk. Risk: Medium Change affects files that have been flagged as medium risk. labels Jun 14, 2024

millerdev force-pushed the dm/pytest branch 6 times, most recently from cf75dbf to e474bf5 Compare June 19, 2024 18:11

millerdev force-pushed the dm/pytest branch from ad120ff to 6525103 Compare July 1, 2024 21:14

millerdev force-pushed the dm/pytest branch 3 times, most recently from a0fb589 to ed40b81 Compare July 26, 2024 15:09

millerdev force-pushed the dm/pytest branch from a6daf9c to 0a9ad06 Compare July 30, 2024 21:02

millerdev force-pushed the dm/pytest branch 6 times, most recently from 13ba100 to 73c0be9 Compare September 4, 2024 16:57

millerdev force-pushed the dm/pytest branch from 73c0be9 to 3c62ebf Compare September 10, 2024 14:55

dimagimon removed Risk: High Change affects files that have been flagged as high risk. Risk: Medium Change affects files that have been flagged as medium risk. labels Sep 10, 2024

millerdev force-pushed the dm/pytest branch 6 times, most recently from 9bc121b to b71a55c Compare September 18, 2024 10:54

millerdev added 13 commits October 23, 2024 16:17

Add 'skip_setup_users' marker

e305b74

Required by --strict-markers

Convert "yield" tests to parametrized tests

68219e5

test_user_types tests were not being run because the "yield" tests were masked by @patch_user_data_db_layer, which converted the generator to a normal function, which was then "run" as a test but didn't actually test anything.

Remove nose test utilities module

5ae9b5e

Align test discovery with nose

c58537c

A few tests are also now found that nose was missing.

Fix parametrized test that hit Couch

214546f

make requirements

2253349

Switch github actions from nose to pytest

e7650e0

- Disable cacheprovider plugin since it is not possible to re-run failed tests in that context. - Add option to show local variables in test failure tracebacks.

Ignore unclosed file warnings in tests

509dddc

Work around pytest capture plugin

05d1a10

Tried using capsys, but couldn't make it work.

Adapt champ tests to pytest

e33e25f

Adapt inddex tests to pytest

cd7c0a8

Adapt up_nrhm tests to pytest

946173a

Update testing documentation: nose -> pytest

ca33bd1

Also a few other non-test files.

millerdev force-pushed the dm/pytest branch from ea429ec to ca33bd1 Compare October 23, 2024 20:23

millerdev marked this pull request as ready for review October 23, 2024 20:49

millerdev requested review from a team, esoergel, AmitPhulera, dannyroberts and orangejenny as code owners October 23, 2024 20:49

millerdev requested review from snopoke and gherceg October 24, 2024 10:37

Merge branch 'master' into dm/pytest

7ebdc62

kaapstorm reviewed Oct 25, 2024

View reviewed changes

biyeun reviewed Oct 31, 2024

View reviewed changes

gherceg approved these changes Nov 5, 2024

View reviewed changes

millerdev merged commit 90bf033 into master Nov 11, 2024
13 checks passed

millerdev deleted the dm/pytest branch November 11, 2024 12:03

This was referenced Nov 22, 2024

Pytest improvements and cleanup #35421

Merged

Skip migrations when running pytest #35447

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace nose with pytest #34778

Replace nose with pytest #34778

millerdev commented Jun 14, 2024 •

edited

Loading

kaapstorm left a comment

kaapstorm Oct 25, 2024

millerdev Oct 25, 2024

kaapstorm Oct 25, 2024

millerdev Oct 25, 2024

kaapstorm Oct 25, 2024

millerdev Oct 28, 2024

kaapstorm Oct 25, 2024

millerdev Oct 25, 2024

kaapstorm Oct 25, 2024

kaapstorm Oct 25, 2024

millerdev Oct 25, 2024

biyeun Oct 31, 2024

millerdev Nov 1, 2024

gherceg left a comment •

edited

Loading

gherceg Nov 5, 2024

millerdev Nov 22, 2024

gherceg Nov 5, 2024

millerdev Nov 22, 2024

gherceg Nov 22, 2024

gherceg Nov 5, 2024

millerdev Nov 22, 2024

		from corehq.apps.es.client import manager


		class ElasticSnitchPlugin(Plugin):

		with pytest.raises(AttributeError, match="Mock object has no attribute 'info'"):
		CouchModel.get_db().info

Replace nose with pytest #34778

Replace nose with pytest #34778

Conversation

millerdev commented Jun 14, 2024 • edited Loading

Safety Assurance

Safety story

Automated test coverage

Rollback instructions

kaapstorm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gherceg left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millerdev commented Jun 14, 2024 •

edited

Loading

gherceg left a comment •

edited

Loading