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

Migrate to pytest or nose2 #104

Open
joshuap opened this issue Feb 16, 2022 · 8 comments
Open

Migrate to pytest or nose2 #104

joshuap opened this issue Feb 16, 2022 · 8 comments

Comments

@joshuap
Copy link
Member

joshuap commented Feb 16, 2022

The "nose" package is no longer maintained, and causes failures on Python 3.10. I did some research, and it seems like there are two recommended options (feel free to make additional suggestions, though):

  1. Migrate to pytest (may be more of a drop-in replacement)
  2. Migrate to nose2

Here are some doc links related to each migration:

https://docs.pytest.org/en/stable/how-to/nose.html
https://docs.nose2.io/en/latest/differences.html

I started the nose -> pytest migration here:

https://github.com/honeybadger-io/honeybadger-python/tree/nose-to-pytest

If you run python setup.py test on that branch, 36 tests are passing, vs. 66 on the main branch. I think that's because of the known issues mentioned here. It may not be discovering all of the test directories, and there may be some unsupported doctests (I'm not sure on that).

@declaresub
Copy link

I might be willing to help with this, as a prelude to adding type hints. I was able to get to 64 passed, 2 skipped, 7 warnings by disabling one failing test. In test_server_payload, the two assertions

assert type(payload['stats']['mem']['total']) == float
assert type(payload['stats']['mem']['free']) == float

are really testing the return value of stats_payload, which varies by platform. On my M1X laptop, psutil is broken, so stats_payload returns {}. This causes the two assertions able to fail. I suggest rewriting that test.

I also suggest a few project changes:

  1. Move to a src/ directory structure. This structure offers several advantages, and no disadvantages that I know of. See (https://blog.ionelmc.ro/2014/05/25/python-packaging/).
  2. Add a requirements.txt file. It was a PITA figuring out what packages need to be installed to run the tests.
  3. Use tox.
  4. Drop support for EOL python versions; this would mean python >=3.7. I understand that there might be business reasons for holding off on this, of course.

@joshuap
Copy link
Member Author

joshuap commented Mar 14, 2022

I might be willing to help with this, as a prelude to adding type hints. I was able to get to 64 passed, 2 skipped, 7 warnings by disabling one failing test. In test_server_payload, the two assertions

assert type(payload['stats']['mem']['total']) == float
assert type(payload['stats']['mem']['free']) == float

are really testing the return value of stats_payload, which varies by platform. On my M1X laptop, psutil is broken, so stats_payload returns {}. This causes the two assertions able to fail. I suggest rewriting that test.

I also suggest a few project changes:

  1. Move to a src/ directory structure. This structure offers several advantages, and no disadvantages that I know of. See (https://blog.ionelmc.ro/2014/05/25/python-packaging/).
  2. Add a requirements.txt file. It was a PITA figuring out what packages need to be installed to run the tests.
  3. Use tox.
  4. Drop support for EOL python versions; this would mean python >=3.7. I understand that there might be business reasons for holding off on this, of course.

I'm good with all of that if you want to take a shot at it. 👍 We're currently building python >= 3.7 in CI, but it looks like I forgot to update the README. I'll do that now.

@declaresub
Copy link

Which versions of django do you want to support? There is at least one change in 4.0 that broke your tests. This handy resource suggests that you would want to support 3.2, 4.x.

@joshuap
Copy link
Member Author

joshuap commented Mar 16, 2022

Which versions of django do you want to support? There is at least one change in 4.0 that broke your tests. This handy resource suggests that you would want to support 3.2, 4.x.

Yep, we only want to support currently maintained releases!

@joshuap joshuap added this to the 0.9.0 milestone Jul 25, 2022
@dotysan
Copy link
Contributor

dotysan commented Jun 2, 2023

Plonk.

@dotysan
Copy link
Contributor

dotysan commented Jun 3, 2023

So over on this tox branch, I've made some progress!

First, I've successfully migrated the unit tests from setup.py to tox.

Then migrated from nose to pytest. All core test pass! Just a couple from contrib/test_django.py and few from contrib/test_celery.py are failing:

FAILED honeybadger/tests/contrib/test_celery.py::CeleryluginTestCase::test_celery_task_with_exception - AssertionError: Expected 'send_notice' to have been called once. Called 0 times.
FAILED honeybadger/tests/contrib/test_celery.py::CeleryluginTestCase::test_celery_task_with_params - AssertionError: Expected 'send_notice' to have been called once. Called 0 times.
FAILED honeybadger/tests/contrib/test_celery.py::CeleryluginTestCase::test_celery_task_with_reset_context - AssertionError: Expected 'send_notice' to have been called once. Called 0 times.
FAILED honeybadger/tests/contrib/test_celery.py::CeleryluginTestCase::test_celery_task_with_retries - AssertionError: Expected 'send_notice' to have been called once. Called 0 times.
FAILED honeybadger/tests/contrib/test_celery.py::CeleryluginTestCase::test_celery_task_without_retries - AssertionError: Expected 'send_notice' to have been called once. Called 0 times.
FAILED honeybadger/tests/contrib/test_django.py::DjangoMiddlewareIntegrationTestCase::test_exceptions_handled_by_middleware - AssertionError: False is not true
FAILED honeybadger/tests/contrib/test_django.py::DjangoMiddlewareIntegrationTestCase::test_exceptions_handled_by_middleware_with_custom_middleware - AssertionError: False is not true

But the best part is that Python 3.10 isn't failing on anything else. And even 3.11 generates only one new error--and it's from the same sketchy CeleryluginTestCase class.

@dotysan
Copy link
Contributor

dotysan commented Jun 3, 2023

So this tox fork/branch was able to successfully build on Python 3.7 to 3.11 using pytest. The only caveat was disabling the 7 oddball tests above.

I don't understand the first thing about mock. If anyone here does, please take a look at those tests please.

@dotysan
Copy link
Contributor

dotysan commented Jun 7, 2023

Check out proof-of-concept PR #153. Feedback please!

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

3 participants