Skip to content

Closes #21766: Add reusable tests for sortable table columns#21836

Merged
jeremystretch merged 1 commit intomainfrom
21766-improve-test-coverage-for-sortable-table-columns
Apr 6, 2026
Merged

Closes #21766: Add reusable tests for sortable table columns#21836
jeremystretch merged 1 commit intomainfrom
21766-improve-test-coverage-for-sortable-table-columns

Conversation

@pheus
Copy link
Copy Markdown
Contributor

@pheus pheus commented Apr 3, 2026

Fixes: #21766

Introduce TableTestCases.OrderableColumnsTestCase, a reusable base class for table ordering smoke tests.

The new helper discovers sortable columns from list-view querysets and verifies that each one renders without exceptions when ordered in both ascending and descending directions. This gives us a consistent way to catch unsupported ordering across model-backed tables without duplicating the same test logic in each app.

This PR also adds per-table smoke tests across circuits, core, dcim, extras, ipam, tenancy, users, virtualization, vpn, and wireless.

@pheus
Copy link
Copy Markdown
Contributor Author

pheus commented Apr 3, 2026

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

@claude
Copy link
Copy Markdown

claude bot commented Apr 3, 2026

Claude finished @pheus's task in 3m 28s —— View job


Code Review

  • Gather context
  • Review changed files
  • Post findings

A few items worth considering:

1. Tests run against empty querysets — ordering bugs with actual data won't be caught

OrderableColumnsTestCase inherits TestCase but never calls setUpTestData or creates any model instances. Every .objects.all() (and the explicitly-provided querysets) return zero rows at test time. Annotation-based columns that evaluate fine against an empty queryset can still blow up in production when rows exist. This is an inherent limitation of the approach — worth calling out in a docstring or comment in utilities/testing/tables.py so future contributors understand the tradeoff.

2. Loss of @tag('regression') on migrated tests

The pre-existing tests in circuits, extras, vpn, and users all carried @tag('regression'). The new OrderableColumnsTestCase-based tests drop that tag entirely. If CI or maintainers ever run manage.py test --tag regression to run only regression checks, these tests will be silently excluded. The new helper could set tags = ('regression',) on the base class, or the consumer classes could apply @tag('regression').

3. Misleading queryset_sources label in FHRPGroupAssignmentTableTest

ipam/tests/test_tables.py:217:

queryset_sources = [
    ('FHRPGroupAssignmentTable', FHRPGroupAssignment.objects.all()),
]

The first element of each tuple is used as the source kwarg in cleanupSubTest, which appears in test failure output. Using the table name as the label is inconsistent with the dcim convention ('ConsoleConnectionsListView') and misleading in failure messages. A label like 'FHRPGroupDetailView' (the view where this table is actually rendered) would be clearer.

4. Fictitious view-name labels for BookmarkTableTest, NotificationTableTest, SubscriptionTableTest

extras/tests/test_tables.py:33, 44, 52 uses labels 'BookmarkListView', 'NotificationListView', 'SubscriptionListView' — none of which exist as actual views (these tables are only used in BulkDeleteView). The labels appear in subtest failure output and will be confusing to anyone debugging a failure who goes looking for those view classes. A label like 'Bookmark.objects.all' or a comment explaining why queryset_sources is manually set would reduce confusion.

5. cleanupSubTest unnecessary for read-only tests

test_every_orderable_column_renders only calls table.as_html(request) — it performs no writes. cleanupSubTest wraps each subtest in a savepoint + rollback (utilities/testing/base.py:44-55), adding overhead for no benefit. A plain with self.subTest(...) would be sufficient and slightly faster across many hundred subtests.


@pheus pheus force-pushed the 21766-improve-test-coverage-for-sortable-table-columns branch from 209c60e to b8b9cf1 Compare April 6, 2026 13:37
@pheus
Copy link
Copy Markdown
Contributor Author

pheus commented Apr 6, 2026

@claude /review

  1. Good point on the empty-queryset limitation. The new helper is mainly intended as a smoke test for unsupported ordering, not as a full data-backed rendering test, so I have made that tradeoff explicit in the helper docstring.

  2. Agreed on the regression tag.

3/4. Also agreed on the manual queryset labels. I’ll rename those to reflect the actual source more clearly, or use neutral queryset labels where there isn’t a real list view.

  1. Good call on cleanupSubTest().

@claude
Copy link
Copy Markdown

claude bot commented Apr 6, 2026

Claude finished @pheus's task in 1m 24s —— View job


Code Review

  • Gather context
  • Review changed files
  • Post findings

All five items from the previous review have been addressed:

  1. Empty-queryset smoke-test limitationtest_every_orderable_column_renders now has an explicit docstring calling this out (utilities/testing/tables.py:131–140). ✅

  2. @tag('regression') restoredCircuitTerminationTableTest, CircuitGroupAssignmentTableTest, EventRuleTableTest, TokenTableTest, and TunnelTerminationTableTest all carry @tag("regression") again. ✅

  3. FHRPGroupAssignmentTableTest label — Now 'FHRPGroupAssignment.objects.all()' with a comment explaining why no ObjectListView exists (ipam/tests/test_tables.py:215–220). ✅

  4. BookmarkTableTest / NotificationTableTest / SubscriptionTableTest labels — Changed to 'Bookmark.objects.all()' etc., each with a comment noting that the list view lives in account.views (extras/tests/test_tables.py:36–55). ✅

  5. cleanupSubTest removed — The inner loop now uses a plain with self.subTest(...) (utilities/testing/tables.py:154). ✅

Nothing further to flag. The helper is clean and the per-app tests are well-structured.

@pheus pheus requested review from a team and jeremystretch and removed request for a team April 6, 2026 13:53
jeremystretch
jeremystretch previously approved these changes Apr 6, 2026
Copy link
Copy Markdown
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

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

Nice work @pheus, really clean! Approving this with two minor administrative points for your consideration.

table = CircuitTable


@tag("regression")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Given that we're moving to a new testing mechanism for tables, IMO it makes sense to remove these tags. (It's safe to ignore Claude's earlier feedback.)

base classes directly.
"""

class OrderableColumnsTestCase(ModelTableTestCase):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we give this a more generic name? Seems reasonable to assume we'll add tests unrelated to ordering in the future.

@pheus
Copy link
Copy Markdown
Contributor Author

pheus commented Apr 6, 2026

That makes sense to me on both points.

I’ll drop the regression tags from the migrated table tests.

I’ll also rename OrderableColumnsTestCase to StandardTableTestCase so the shared class has room to grow as we add more common table tests in the future.

Introduce `TableTestCases.StandardTableTestCase`, a shared base class
for model-backed table smoke tests. It currently discovers sortable
columns from list-view querysets and verifies that each renders without
exceptions in both ascending and descending order.

Add per-table smoke tests across circuits, core, dcim, extras, ipam,
tenancy, users, virtualization, vpn, and wireless apps.

Fixes #21766
@pheus pheus force-pushed the 21766-improve-test-coverage-for-sortable-table-columns branch from b8b9cf1 to 19e21ba Compare April 6, 2026 17:25
@pheus pheus requested a review from jeremystretch April 6, 2026 17:42
@jeremystretch jeremystretch merged commit d6a1cc5 into main Apr 6, 2026
11 checks passed
@jeremystretch jeremystretch deleted the 21766-improve-test-coverage-for-sortable-table-columns branch April 6, 2026 17:53
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.

Improve test coverage for sortable table columns

2 participants