Opened 23 months ago

Closed 22 months ago

Last modified 17 months ago

#32178 closed New feature (fixed)

Allow database backends to skip tests and mark expected failures

Reported by: Tim Graham Owned by: Hasan Ramezani
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently Django's built-in database backends have special privileges in the test suite when it comes to test skipping such as:
tests/model_fields/test_integerfield.py: @unittest.skipIf(connection.vendor == 'sqlite', "SQLite doesn't have a constraint.")

While DatabaseFeatures attributes are generally preferred to connection.vendor checking, I think allowing backends to skip certain tests has its place. Adding features flags that are likely to apply to only a single database is a bit cumbersome, especially for third-party backends.

Django should provide more generic way to skip tests and mark expected failures. For example, see django-cockroachdb's logic which was inspired by django-mssql-backend. In a more refined version, expected_failures and skip_classes would be attributes of DatabaseFeatures.

That logic uses a RUNNING_COCKROACH_BACKEND_TESTS environment variable to decide whether or not the Django test suite is running. There might be a better way to determine that. At least the variable name would be more generic.

Change History (19)

comment:1 Changed 23 months ago by Mariusz Felisiak

Triage Stage: UnreviewedAccepted

comment:2 Changed 23 months ago by Hasan Ramezani

In a more refined version, expected_failures and skip_classes would be attributes of DatabaseFeatures.

What can we do to skip a single test? I checked the django-cockroachdb and there you have a skip_tests which can be used to skip a single test(If I got it right).

comment:3 Changed 23 months ago by Tim Graham

Yes, skip_tests should also be a database feature attribute.

Last edited 23 months ago by Tim Graham (previous) (diff)

comment:4 Changed 22 months ago by Hasan Ramezani

Hi Tim,

I created a PR based on django-cockroachdb example. Please check the PR if you have time and if you confirm it, I will continue with moving skip tests to database creation files.

I added expected_failures_tests and skip_tests(which can be used for skipping both test class and function) and added a mark_tests method similar to the one that you did for django-cockroachdb.

comment:5 Changed 22 months ago by Hasan Ramezani

Has patch: set
Owner: changed from nobody to Hasan Ramezani
Status: newassigned

comment:6 Changed 22 months ago by Tim Graham

Patch needs improvement: set

I'll mark this as RFC when I'm done iterating with Hasan. (Meanwhile, let's leave "needs improvement" checked so it doesn't appear in the review queue for others.)

comment:7 Changed 22 months ago by Hasan Ramezani

Patch needs improvement: unset

comment:8 Changed 22 months ago by Mariusz Felisiak

Needs documentation: set
Patch needs improvement: set

comment:9 Changed 22 months ago by Hasan Ramezani

Needs documentation: unset
Patch needs improvement: unset

comment:10 Changed 22 months ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:11 Changed 22 months ago by GitHub <noreply@…>

Resolution: fixed
Status: assignedclosed

In 275dd4eb:

Fixed #32178 -- Allowed database backends to skip tests and mark expected failures.

Co-authored-by: Tim Graham <timograham@…>

comment:12 Changed 22 months ago by GitHub <noreply@…>

In 3f140dde:

Refs #32178 -- Changed BaseDatabaseFeatures.django_test_expected_failures to set().

comment:13 Changed 22 months ago by GitHub <noreply@…>

In 1e765311:

Refs #32178 -- Fixed test_mark_expected_failures_and_skips_call teardown.

Test isolation failure observed on CockroachDB and PostgreSQL.

comment:14 Changed 20 months ago by Chris Jerdonek

I have a comment / question about this feature. I was analyzing a test by inspecting the code, and I couldn't figure out why it was passing on SQLite because it used to have "skip" code which is no longer there. And then I came across this ticket via the history.

Would it be possible to have at least a code convention of including a code comment at the test site when the test is included in django_test_skips? Otherwise, the average reader wouldn't know to check there for the test's name. (I would call this behavior "magical" because it's not explicit at the code site.) Also, even for people that know about it, it's not something that's easy to check while e.g. browsing the code on the web.

comment:15 Changed 20 months ago by Tim Graham

I sympathize but would you give third-party backends the same privilege? If so, it may be better to scrap this feature and allow third-parties to contribute their skips directly in the test suite. If not, this feature could at least be documented in the contributing guide.

comment:16 Changed 17 months ago by Hasan Ramezani

I've created a PR to add them to the contributing guide.

comment:17 Changed 17 months ago by Hasan Ramezani

Triage Stage: Ready for checkinAccepted

comment:18 Changed 17 months ago by GitHub <noreply@…>

In ca34db46:

Refs #32178 -- Doc'd DatabaseFeatures.django_test_skips/django_test_expected_failures in contributing guide.

comment:19 Changed 17 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In ce130749:

[3.2.x] Refs #32178 -- Doc'd DatabaseFeatures.django_test_skips/django_test_expected_failures in contributing guide.

Backport of ca34db46504fca1221e27f6ab13734dfdfde6e1c from main

Note: See TracTickets for help on using tickets.
Back to Top