Skip to content

Remove outdated pins #2690

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

Merged
merged 63 commits into from
Apr 10, 2024
Merged

Remove outdated pins #2690

merged 63 commits into from
Apr 10, 2024

Conversation

sentrivana
Copy link
Contributor

@sentrivana sentrivana commented Jan 30, 2024

Remove version pins on tools that are no longer necessary.

Relates #2586


General Notes

Thank you for contributing to sentry-python!

Please add tests to validate your changes, and lint your code using tox -e linters.

Running the test suite on your PR might require maintainer approval. Some tests (AWS Lambda) additionally require a maintainer to add a special label to run and will fail if the label is not present.

For maintainers

Sensitive test suites require maintainer review to ensure that tests do not compromise our secrets. This review must be repeated after any code revisions.

Before running sensitive test suites, please carefully check the PR. Then, apply the Trigger: tests using secrets label. The label will be removed after any code changes to enforce our policy requiring maintainers to review all code revisions before running sensitive tests.

@sentrivana sentrivana self-assigned this Jan 30, 2024
@sentrivana sentrivana linked an issue Feb 5, 2024 that may be closed by this pull request
@sentrivana sentrivana changed the title Remove outdated pins Remove outdated pins from test-requirements Apr 3, 2024
@sentrivana sentrivana removed a link to an issue Apr 3, 2024
@sentrivana sentrivana mentioned this pull request Apr 3, 2024
@antonpirker
Copy link
Member

I just ran the tests locally and there is no error. Just the tests/integrations/django/asgi/test_asgi.py tests in the Django test suite freeze and thus the test suite times out. So something with async stuff is off.

@sentrivana
Copy link
Contributor Author

I just ran the tests locally and there is no error. Just the tests/integrations/django/asgi/test_asgi.py tests in the Django test suite freeze and thus the test suite times out. So something with async stuff is off.

Yeah this. Also works for me locally. Interestingly, it only fails on 3.12, but it's not some dependency issue because it installs the same versions of the same dependencies on both 3.11 (where it's working) and 3.12.

Will try to bisect a bit what is causing this.

@antonpirker
Copy link
Member

I just messed around with the Django tests. The problem is the following:

  • We have this "Web Frameworks 1" test suite that runs multiple test suites in a matrix.
  • Every test suite uses the service "postgres" defined in the yaml file.
  • And in this postgres service we create one database called "ci_test"
  • When the first test that is decorator with pytest_mark_django_db_decorator is run, the database is filled by running the django migrations.

And my guess now is that when two test suites run at the same time they both want to fill the db by running the migrations and because there is already something in the db, it fails with the duplicate key value violates unique constraint error.

And as so often when fixing bugs the question is: How has this ever worked at some point? :-)

Solution would now be to either have one postgres server by test suite, or one unique database name per test suite. (or maybe create the postgres server in some completely different way)

@antonpirker
Copy link
Member

I played around more and have this: https://github.com/getsentry/sentry-python/actions/runs/8572070273/job/23493776173?pr=2938

The thing now is that in "Setup Test Env" i try to delete the uniquely named db name but it does not exist. (which is fine) Two steps later when running the django tests they fail becaus the db is already existing. So the db is not cleaned up between running different django versions in one call to tox. Not sure why, but this is something for future Anton!

@sentrivana
Copy link
Contributor Author

@antonpirker Maybe this can be somehow combined with --create-db/--reuse-db? https://pytest-django.readthedocs.io/en/latest/database.html#create-db-force-re-creation-of-the-test-database

--create-db - force re creation of the test database

When used with --reuse-db, this option will re-create the database, regardless of whether it exists or not.

@antonpirker
Copy link
Member

I think I have solved the problem now in my PR: #2938
I removed the whole setting of the db name in CI with env variables and just let the django app itself set the database name (including a random number) this way we always have one db with a unique name for a testrun that is also cleaned up by pytest-django. if the tests pass, I will merge my PR into this one

@antonpirker
Copy link
Member

Ok, the Python 3.12 tests still freeze, but the rest is green so I will merge it into here.

@sentrivana
Copy link
Contributor Author

Awesome! The Web Frameworks 1 tests on 3.12 will still time out but that's not a problem with your PR. Everything else seems green so feel free to merge

antonpirker and others added 6 commits April 8, 2024 11:25
Make sure the Django tests are always run with a unique database name that is cleaned up after the tests have been run.
@sentrivana
Copy link
Contributor Author

sentrivana commented Apr 8, 2024

Regarding the freezing 3.12 tests: the problem goes away as soon as you remove tox from test-requirements.txt. Which I think we can do -- the purpose of test-requirements seems to be to set up the test env after tox has already created it.

@sentrivana sentrivana marked this pull request as ready for review April 8, 2024 10:13
@sentrivana
Copy link
Contributor Author

Marking this as ready for review but we shouldn't merge this until the final release is out.

@sentrivana sentrivana linked an issue Apr 8, 2024 that may be closed by this pull request
@sentrivana sentrivana merged commit 467bde9 into sentry-sdk-2.0 Apr 10, 2024
@sentrivana sentrivana deleted the ivana/2.0/update-tooling branch April 10, 2024 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update tooling
2 participants