Skip to content

Removed skipIf and fixed failing test#214

Open
dnlzrgz wants to merge 2 commits intodjango-cms:masterfrom
dnlzrgz:remove-skip-if
Open

Removed skipIf and fixed failing test#214
dnlzrgz wants to merge 2 commits intodjango-cms:masterfrom
dnlzrgz:remove-skip-if

Conversation

@dnlzrgz
Copy link
Contributor

@dnlzrgz dnlzrgz commented Mar 14, 2026

Description

Removed @skipIf(cms_version) decorators and unnecessary imports. I also fixed a test that failed after removing the decorator (test_admin.SnippetAdminTestCase.test_admin_list_display_with_versioning).

Related resources

Checklist

  • I have opened this pull request against master
  • I have added or modified the tests when changing logic
  • I have followed the conventional commits guidelines to add meaningful information into the changelog
  • I have read the contribution guidelines and I have joined #pr-review on
    Discord to find a “pr review buddy” who is
    going to review my pull request.

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Mar 14, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Removes version-dependent @skipIf decorators from snippet-related tests so they always run, and updates the versioning admin list_display test to match the current ExtendedIndicatorVersionAdminMixin-based implementation and dynamic indicator field behaviour.

File-Level Changes

Change Details Files
Align snippet admin list_display versioning test with current admin mixin and indicator behaviour.
  • Remove cms_version-based @skipIf decorator from versioning-enabled list_display test so it always runs.
  • Update expected admin base classes to assert ExtendedIndicatorVersionAdminMixin instead of the older mixin.
  • Relax list_display expectations to assert static fields explicitly, validate that the indicator field is a callable named 'indicator', and keep verifying that the last entry is the actions column with the correct description.
tests/test_admin.py
Remove cms_version-based skipping from snippet admin tests that depend on versioning behaviour.
  • Drop @skipIf(cms_version < '4', ...) decorators from snippet admin tests around delete button visibility, delete endpoint accessibility, form save behaviour with versioning, and editing when locked so they always execute under the configured settings.
  • Keep per-test @override_settings(DJANGOCMS_SNIPPET_VERSIONING_ENABLED=...) to control versioning behaviour explicitly.
tests/test_admin.py
Clean up spelling in a test name for clarity and consistency.
  • Rename test_slug_colomn_should_hyperlinked_with_versioning_disabled to test_slug_column_should_hyperlinked_with_versioning_disabled to fix the 'colomn' typo.
tests/test_admin.py
Ensure migration and form tests always run by removing Django CMS version-based skips.
  • Remove the @skipIf decorator from MigrationTestCase.test_for_missing_migrations so the missing-migrations check always runs, leaving override_settings(MIGRATION_MODULES={}) in place.
  • Drop @skipIf(cms_version < '4', ...) from form tests that cover versioning interactions and validation with multiple version states in a grouper so they run regardless of cms_version.
tests/test_migrations.py
tests/test_forms.py
Remove cms_version-based skipping from config and plugin versioning tests.
  • Delete @skipIf(cms_version < '4', ...) from VersioningConfigTestCase so copy behaviour is always tested.
  • Delete @skipIf(cms_version < '4', ...) from SnippetPluginVersioningRenderTestCase so plugin rendering under versioning is always exercised.
tests/test_config.py
tests/test_plugins.py
Drop now-unused test-only imports tied to cms_version-based skipping.
  • Remove skipIf and cms_version imports from tests that no longer need them after dropping version-gated decorators.
tests/test_migrations.py
tests/test_forms.py
tests/test_config.py
tests/test_plugins.py
tests/test_admin.py

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • The assertions in test_admin_list_display_with_versioning about the exact base class tuple (ExtendedIndicatorVersionAdminMixin, admin.ModelAdmin) tightly couple the test to the current inheritance order; consider using issubclass/isinstance checks or asserting presence instead of exact tuple equality to make the test more robust to internal refactors.
  • Similarly, asserting indicator_field.__name__ == "indicator" in test_admin_list_display_with_versioning makes the test sensitive to a purely internal naming detail; you could instead assert on behavior (e.g., that the callable is present in the expected position and returns the right value for a sample object) to reduce brittleness.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The assertions in `test_admin_list_display_with_versioning` about the exact base class tuple `(ExtendedIndicatorVersionAdminMixin, admin.ModelAdmin)` tightly couple the test to the current inheritance order; consider using `issubclass`/`isinstance` checks or asserting presence instead of exact tuple equality to make the test more robust to internal refactors.
- Similarly, asserting `indicator_field.__name__ == "indicator"` in `test_admin_list_display_with_versioning` makes the test sensitive to a purely internal naming detail; you could instead assert on behavior (e.g., that the callable is present in the expected position and returns the right value for a sample object) to reduce brittleness.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

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.

1 participant