Skip to content

Refs #36416, #34378, #36143 -- Fixed isolation of LookupTests.test_in_bulk_preserve_ordering_with_batch_size(). #19532

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 9, 2025
Merged

Refs #36416, #34378, #36143 -- Fixed isolation of LookupTests.test_in_bulk_preserve_ordering_with_batch_size(). #19532

merged 1 commit into from
Jun 9, 2025

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Jun 6, 2025 •
edited
Loading

max_query_params is a property on SQLite since ticket-36143 (6.0), so the test for ticket-34378 must patch the class.

This was exposed via a poor interaction with the test for ticket-36416 (backported to 5.2, but rewritten in #19522 for 6.0, ticket-36430).

To reproduce the failure on postgres, here is a django-docker-box invocation:

docker compose run --rm postgresql -k bulk_preserve -k batching --parallel=1 --reverse

test_in_bulk_preserve_ordering_with_batch_size (lookup.tests.LookupTests.test_in_bulk_preserve_ordering_with_batch_size) ... ok
test_in_bulk_preserve_ordering (lookup.tests.LookupTests.test_in_bulk_preserve_ordering) ... ok
test_in_bulk_batching (composite_pk.tests.CompositePKTests.test_in_bulk_batching) ... ERROR

======================================================================
ERROR: test_in_bulk_batching (composite_pk.tests.CompositePKTests.test_in_bulk_batching)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python3.12/unittest/mock.py", line 1396, in patched
    return func(*newargs, **newkeywargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/django/source/tests/composite_pk/tests.py", line 155, in test_in_bulk_batching
    num_requiring_batching = (connection.features.max_query_params // 2) + 1
                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^~~
TypeError: unsupported operand type(s) for //: 'NoneType' and 'int'

----------------------------------------------------------------------
Ran 3 tests in 0.039s

FAILED (errors=1)

No need to backport, since max_query_params is a property only on main. However, I think this change still brings value separately from #19522: if you adjust test_in_bulk_preserve_ordering_with_batch_size() to run on SQLite by removing the skip decorator, you see:

======================================================================
ERROR: test_in_bulk_preserve_ordering_with_batch_size (lookup.tests.LookupTests.test_in_bulk_preserve_ordering_with_batch_size)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jwalls/django/tests/lookup/tests.py", line 264, in test_in_bulk_preserve_ordering_with_batch_size
    connection.features.max_query_params = 1
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: property 'max_query_params' of 'DatabaseFeatures' object has no setter

----------------------------------------------------------------------

@jacobtylerwalls jacobtylerwalls changed the title Refs #36416, Refs #34378, Refs #36143 -- Fixed isolation of LookupTests.test_in_bulk_preserve_ordering_with_batch_size(). Refs #36416, #34378, #36143 -- Fixed isolation of LookupTests.test_in_bulk_preserve_ordering_with_batch_size(). Jun 6, 2025
@jacobtylerwalls @nessita
Refs #34378, #36143, #36416 -- Fixed isolation of LookupTests.test_in…
b70db1f 
…_bulk_preserve_ordering_with_batch_size().

`max_query_params` is a property, so it must be patched on the class.
@nessita nessita force-pushed the fix-test-patching branch from 3dd0efe to b70db1f Compare June 9, 2025 15:48
nessita
nessita approved these changes Jun 9, 2025
View reviewed changes
Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

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

Thank you, looks great! I pushed a small tweak.

@nessita nessita merged commit a68e856 into django:main Jun 9, 2025
30 checks passed
@jacobtylerwalls jacobtylerwalls deleted the fix-test-patching branch June 9, 2025 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nessita nessita nessita approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jacobtylerwalls @nessita