Problem/Motivation

In #2350823: Use the Classy theme in the Testing profile we made tests use Classy as their theme. Kernel tests should not expect a theme to be set. If a theme is needed, the test should set a default theme.

Proposed resolution

Remove the default theme from KernelTestBase

Remaining tasks

Test patch

User interface changes

None

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because Drupal works without this change.
Unfrozen changes Unfrozen because it only changes tests.
CommentFileSizeAuthor
#6 2409811.6.patch829 bytesalexpott
#1 use_the_classy_theme_as-2409811-1.patch2.67 KBlauriii

Comments

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Active » Needs review
StatusFileSize
new use_the_classy_theme_as-2409811-1.patch2.67 KB

Start for this patch

Status: Needs review » Needs work

The last submitted patch, 1: use_the_classy_theme_as-2409811-1.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I understand we want to be able to test Classy. But let us make sure this is the right solution; it could be argued that if you want to test Classy-based markup, you have to write Kernel tests using Classy.

The guideline has been to test with Stark for a good reason: to test everything in an as bare/minimal as possible form. But I think it was to test with minimal *CSS*, not with minimal *markup*, right? If it was about testing with minimal markup, then the change to Classy would be okay, I think.
That being said: what would still be the use of Stark once we use Classy for the purpose Stark was designed for? It sounds to me like Stark should go away, because Classy supersedes it?

Ideally, we'd get sun's input on this.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

what would still be the use of Stark once we use Classy for the purpose Stark was designed for?

Stark would never go away, because it will always be the way to receive core module templates that are not being overridden by Classy.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#4: right, right, stupid me! I think the rest of my comment still stands though.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 2409811.6.patch829 bytes

So I think the reason for the theme hack in KernelTestBase was because of how drupal_render used to work. It works different now and so is unnecessary.

Any KernelTestBase test that needs something specific from a theme should just install the theme in that test's setUp and set the default theme.

cilefen’s picture

cilefen commented
Title: Use the Classy theme as a theme for Kernel tests » Don't use the Classy theme as a theme for Kernel tests

I think this is the correct title.

cilefen’s picture

cilefen commented
Status: Needs review » Reviewed & tested by the community

I agree with this issue 100%. It doesn't interfere with tests that extend KernelTestBase but also enable themes, such as CKEditorTest, which enables bartik.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Title: Don't use the Classy theme as a theme for Kernel tests » Kernel tests should explicitly install themes
Issue tags: +Needs issue summary update

The issue summary and title were describing the old approach, I think this new title better explains things?

humansky’s picture

humansky commented
Issue summary: View changes
cilefen’s picture

cilefen commented
Category: Bug report » Task
Issue summary: View changes
almaudoh’s picture

almaudoh commented
Component: theme system » simpletest.module

Based on patch and title, this is really a simpletest issue.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed a34f0db on 8.0.x 
    Issue #2409811 by lauriii, alexpott: Kernel tests should explicitly...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.