Skip to content

Fix CI test to actually use the Postgres database#9954

Open
browniebroke wants to merge 14 commits intoencode:mainfrom
browniebroke:fix/database-url-ci
Open

Fix CI test to actually use the Postgres database#9954
browniebroke wants to merge 14 commits intoencode:mainfrom
browniebroke:fix/database-url-ci

Conversation

@browniebroke
Copy link
Copy Markdown
Member

@browniebroke browniebroke commented Apr 7, 2026

Description

After merging #9949, I realised that the setup wasn't complete and Postgres tests are in fact skipped:

SKIPPED [7] tests/test_filters.py: Requires PostgreSQL database backend

This is because we run tests with tox and it needs to be instructed to pass environment variables down with the pass_env option.

Implementation notes

  1. Added pass_env to the main test env.
  2. The testenv:base runs without optional deps (without psycopg), so I unset the pass_env from the main test env.
  3. Once this was set, the tests started failing with psycopg.errors.UndefinedTable: relation "auth_user" does not exist, which I fixed by passing --no-migrations to pytest.
  4. A number of tests started failing due to ordering assumptions which were no longer true, added explicit .order_by() to fix them.
  5. Some tests had hardcoded IDs on the assertions, and fixing them properly made the tests a lot less readable and the diff quite big. I've introduced a fixture to reset the sequences on postgres for these. The fixture slows down a bit the test suite, so it's applied only as needed (instead of auto-using for the whole test suite).

Blocks #9385

@browniebroke browniebroke marked this pull request as ready for review April 28, 2026 21:02
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR completes the CI/PostgreSQL test setup by ensuring tox passes DATABASE_URL through, adjusts pytest execution to avoid migration-related failures, and makes test behavior deterministic across database backends (notably PostgreSQL sequence behavior and queryset ordering).

Changes:

  • Configure tox to pass DATABASE_URL into test environments so PostgreSQL-required tests are exercised.
  • Run pytest with --no-migrations and adjust tests/fixtures to avoid Postgres-specific failures (missing tables, sequence/PK differences).
  • Stabilize tests by enforcing deterministic queryset ordering and backend-aware validator expectations.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tox.ini Passes DATABASE_URL into tox environments to enable Postgres-backed test runs.
pyproject.toml Adds --no-migrations to default pytest options to avoid migration/setup issues.
tests/conftest.py Adds an autouse fixture to reset Postgres sequences between tests for predictable PKs.
tests/test_validators.py Makes validator-count expectation backend-aware using connection.ops.integer_field_range.
tests/test_relations_slug.py Adds order_by('pk') to avoid nondeterministic ordering across DBs.
tests/test_relations_pk.py Adds order_by('pk') to avoid nondeterministic ordering across DBs.
tests/test_relations_hyperlink.py Adds order_by('pk') to avoid nondeterministic ordering across DBs.
tests/test_filters.py Increases a CharField(max_length=...) to avoid Postgres-enforced length failures.
rest_framework/test.py Attempts to prevent DB connection closing during RequestsClient WSGI calls by mutating Django signal handlers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tox.ini
Comment thread tests/conftest.py
Comment thread rest_framework/test.py Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread rest_framework/test.py
Comment on lines +34 to +35
request_started.disconnect(close_old_connections)
request_finished.disconnect(close_old_connections)
Copy link

Copilot AI May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_keep_connections_open() disconnects close_old_connections and then unconditionally reconnects it in finally. This can mutate global signal state if the receiver was not connected to begin with (or if the context is nested), and it is not thread-safe because signals are process-global. Track whether each disconnect actually removed a receiver (and only reconnect when needed), and consider making the context re-entrant (eg, a counter) to avoid inner contexts re-enabling connection-closing while an outer context is still active.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

@browniebroke browniebroke May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand when that would be a problem in practice... As far as I understand, this is mainly used when running tests, and Django --parrallel test option runs each worker in separate processes, not threads (source).

Django itself does this disconnect/connect in ClientHandler

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants