sentry-python icon indicating copy to clipboard operation
sentry-python copied to clipboard

Instructions in `CONTRIBUTING.md` lead to an incomplete environment

Open arr-ee opened this issue 1 year ago • 2 comments

How do you use Sentry?

Self-hosted/on-premise

Version

1.40.5

Steps to Reproduce

  1. Clone repo
  2. Run steps outlined in CONTRIBUTING.md#Development Environment

Expected Result

pytest tests/ comes up all green

Actual Result

A string of small issues that leads from tests failing to even start to incompatible dependencies version to failures that should not be there.

  1. linter-requirements.txt installs pymongo, which unmasks pymongo integration tests. They rely on mockupdb being installed, but it's only mentioned in tox.ini => pytest blows up on collection. After installing mockupdb, I get 4 errors and 18 warnings (linux aarch64).
  2. Errors are due to test-requirements.txt not constrainting pytest version, triggering bug in pytest-forked that is documented in tox.ini
  3. Majority of warnings are due to pytest-asyncio not being installed by anything except for tox (and installing it without version constraints from tox.ini causes exciting issues)

None of the issues above are serious, but combined they add quite a bit of annoyance to the (ideally) first step of contributing to the project: developer environment setup.

Given the number of additional constraints mentioned in tox.ini, I almost wonder if suggesting using it instead of just pytest might be a good idea (btw tox just worked for me — great job setting it up!)

There is a quick (but brittle) solution, which is to introduce a Yet Another Requirements File for dev env bringup (and maybe make it pull others internally), but if anyone on the team has Thoughts on dependency management management but no time to implement them I'd be happy to lend a hand.

arr-ee avatar Feb 25 '24 06:02 arr-ee

A couple other things related to the test suite that do not warrant separate issues (yet):

  • eventlet tests appear to be dead since they're never ran in CI, so explicitly marking them as such (or removing them from conftest) might be a good call. I spent way too much time wondering why my interpreter is segfaulting and/or hanging while I was unskipping non-integration tests.

  • GCP tests are not excluded from common test suite, which is a shame since they are by far the slowest part of it:

    ==== slowest 10 durations ====
    15.08s call     tests/integrations/gcp/test_gcp.py::test_timeout_error
    8.77s call     tests/integrations/gcp/test_gcp.py::test_handled_exception
    7.17s call     tests/integrations/gcp/test_gcp.py::test_error_has_new_trace_context_performance_disabled
    7.12s call     tests/integrations/gcp/test_gcp.py::test_error_has_existing_trace_context_performance_disabled
    6.25s call     tests/integrations/gcp/test_gcp.py::test_unhandled_exception
    5.13s call     tests/integrations/gcp/test_gcp.py::test_traces_sampler_gets_correct_values_in_sampling_context
    5.10s call     tests/integrations/gcp/test_gcp.py::test_performance_error
    5.08s call     tests/integrations/gcp/test_gcp.py::test_error_has_new_trace_context_performance_enabled
    5.08s call     tests/integrations/gcp/test_gcp.py::test_error_has_existing_trace_context_performance_enabled
    5.06s call     tests/integrations/gcp/test_gcp.py::test_performance_no_error
    ==== 1016 passed, 143 skipped, 4 warnings in 104.32s (0:01:44) ====
    

    Both excluding them from common and optimising tests themselves (maybe rebuilding the project for each test is not strictly necessary?) should shave off some seconds from several CI jobs. Unless current behaviour is by design, I can take a stab at it.

arr-ee avatar Feb 25 '24 06:02 arr-ee

Hey @arr-ee, thanks for opening this -- this is all very valuable feedback. Setting up the test suite is currently far from ideal to the point where it's sometimes easier to just submit a PR and have the CI run the tests instead. 🙈

Feel free to suggest improvements to the contributing guide -- I agree that suggesting to use tox directly is a good idea since that's the closest you get to a "standardized" environment that sets almost everything up for you.

As for your other two points:

  • eventlet tests not being run: I noticed this before but didn't investigate if that was intentional or just an oversight -- thanks for finding the relevant commit. We might as well remove the tests and remove eventlet from any fixtures.
  • Excluding gcp tests from common: also sounds good to me. As for not rebuilding the project for each test, haven't looked into this so not sure if reusing stuff can't have some side effects, but if not, also sounds good.

At the moment the biggest bottleneck is https://github.com/getsentry/sentry-python/issues/2721 though -- so any smaller gains won't be super noticeable. (Not that we shouldn't do them though. :))

sentrivana avatar Feb 27 '24 11:02 sentrivana

This issue should be fixed by #2761

szokeasaurusrex avatar Jun 03 '24 19:06 szokeasaurusrex