Opened 3 years ago

Closed 3 years ago

Last modified 3 years 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 by Mariusz Felisiak, 3 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Hasan Ramezani, 3 years ago

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 by Tim Graham, 3 years ago

Yes, skip_tests should also be a database feature attribute.

Last edited 3 years ago by Tim Graham (previous) (diff)

comment:4 by Hasan Ramezani, 3 years ago

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 by Hasan Ramezani, 3 years ago

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

comment:6 by Tim Graham, 3 years ago

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 by Hasan Ramezani, 3 years ago

Patch needs improvement: unset

comment:8 by Mariusz Felisiak, 3 years ago

Needs documentation: set
Patch needs improvement: set

comment:9 by Hasan Ramezani, 3 years ago

Needs documentation: unset
Patch needs improvement: unset

comment:10 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:11 by GitHub <noreply@…>, 3 years ago

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 by GitHub <noreply@…>, 3 years ago

In 3f140dde:

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

comment:13 by GitHub <noreply@…>, 3 years ago

In 1e765311:

Refs #32178 -- Fixed test_mark_expected_failures_and_skips_call teardown.

Test isolation failure observed on CockroachDB and PostgreSQL.

comment:14 by Chris Jerdonek, 3 years ago

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 by Tim Graham, 3 years ago

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 by Hasan Ramezani, 3 years ago

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

comment:17 by Hasan Ramezani, 3 years ago

Triage Stage: Ready for checkinAccepted

comment:18 by GitHub <noreply@…>, 3 years ago

In ca34db46:

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

comment:19 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

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