WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#29925 closed defect (bug) (fixed)

Setting WP_DEFAULT_THEME to a custom theme causes test to fail

Reported by: mboynes Owned by: jorbin
Milestone: 4.1 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords:
Focuses: Cc:

Description

Today I ran core's unit tests with Underscores active and the following test failed:

There was 1 failure:

1) Tests_Theme::test_default_themes_have_textdomain
Failed asserting that an array contains '_s'.

/var/www/wordpress-develop/tests/phpunit/tests/theme.php:166

This test is directly asserting that WP_DEFAULT_THEME is one of the included themes. I see no reason for this, especially in this test which offers no reference to WP_DEFAULT_THEME and is checking that the default themes have a textdomain.

Attachments (2)

29925.diff (485 bytes) - added by mboynes 6 years ago.
Remove assertion that WP_DEFAULT_THEME is one of the provided themes
29925-cs.diff (646 bytes) - added by TobiasBg 6 years ago.

Download all attachments as: .zip

Change History (10)

@mboynes
6 years ago

Remove assertion that WP_DEFAULT_THEME is one of the provided themes

#1 @nacin
6 years ago

So, it is there for a reason, believe it or not. I think I wrote this one.

When we add a new theme, we update WP_DEFAULT_THEME. This isn't something we'll forget. But it's very possible that we'll forget to update the $default_themes property of the Tests_Theme class. That's why this assertion is there.

I'm generally not against adjusting poor assumptions in our unit tests, but we also can't make a guarantee that they'll always pass when the default environment is changed.

#2 @mboynes
6 years ago

Thanks for the insight @nacin!

What if we added a filter to $this->default_themes?

I think there's a good use case for allowing someone to test their theme against the full suite of core unit tests, to ensure that customizations in the theme don't break core features. I'm open to other solutions to help accomplish that end goal.

#3 @nacin
6 years ago

I think we could break this into a separate test, and only do the assertion if WP_DEFAULT_THEME starts with "twenty*", and otherwise skip the test.

#4 @jorbin
6 years ago

  • Milestone changed from Awaiting Review to 4.1
  • Owner set to jorbin
  • Status changed from new to assigned

Nacin's suggestion seems like the right fix for this situation.

#5 @jorbin
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 29946:

Check if WP_DEFAULT_THEME starts with twenty before asserting it is in default theme array

This fixes an issue that if you change WP_DEFAULT_THEME and run core unit tests, the tests fail since your theme isn't one of the hard coded lists of default themes. We need to keep this test to make sure that we update the array of default themes for use in other tests.

If we ever change the naming convention for default themes, this will need to be updated.

props nacin for initial idea
fixes #29925

#6 @TobiasBg
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Let's maintain coding standards and white space for new commits, see patch.

Last edited 6 years ago by TobiasBg (previous) (diff)

@TobiasBg
6 years ago

#7 @SergeyBiryukov
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 29952:

Fix formatting in [29946].

props TobiasBg.
fixes #29925.

#8 @dd32
4 years ago

In 39065:

Themes: Update the unit tests to handle [39064] and #31550.

test_default_theme_in_default_theme_list() was always being skipped after #31550, this causes it to once again check that the unit tests are up to date and include the latest default theme.
test_default_themes_have_textdomain() didn't play happy when a default theme wasn't installed on a site.

See #31550, #29925, #38551.

Note: See TracTickets for help on using tickets.