Use the Classy theme in the Testing profile

Here is what this would look like I think?

We'd need a followup to remove this dependency (which we reference in the patch). ;) We might also need followups to look at these tests that specifically mention Spark to ensure they're not losing coverage:

core/modules/block/src/Tests/BlockConfigSchemaTest.php
core/modules/block/src/Tests/BlockRenderOrderTest.php
core/modules/block/src/Tests/BlockStorageUnitTest.php
core/modules/block/src/Tests/BlockTest.php
core/modules/block/src/Tests/BlockUiTest.php
core/modules/block/src/Tests/BlockViewBuilderTest.php
core/modules/block/src/Tests/NewDefaultThemeBlocksTest.php
core/modules/block/src/Tests/Views/DisplayBlockTest.php
core/modules/block_content/src/Tests/BlockContentTypeTest.php
core/modules/comment/src/Tests/CommentLinksTest.php
core/modules/config/src/Tests/ConfigImportUITest.php
core/modules/language/src/Tests/LanguageBlockSettingsVisibilityTest.php
core/modules/menu_ui/src/Tests/MenuCacheTagsTest.php
core/modules/search/src/Tests/SearchBlockTest.php
core/modules/system/src/Tests/Block/SystemMenuBlockTest.php
core/modules/system/src/Tests/Bootstrap/GetFilenameUnitTest.php
core/modules/system/src/Tests/Bootstrap/PageCacheTest.php
core/modules/system/src/Tests/Entity/EntityCacheTagsTestBase.php
core/modules/system/src/Tests/Entity/EntityCrudHookTest.php
core/modules/system/src/Tests/Entity/EntityWithUriCacheTagsTestBase.php
core/modules/system/src/Tests/Extension/ThemeHandlerTest.php
core/modules/system/src/Tests/Installer/InstallerTranslationTest.php
core/modules/system/src/Tests/Menu/MenuRouterTest.php
core/modules/system/src/Tests/System/ThemeTest.php
core/modules/system/src/Tests/Theme/ThemeEarlyInitializationTest.php
core/modules/system/src/Tests/Theme/ThemeSettingsTest.php
core/modules/tour/src/Tests/TourCacheTagsTest.php
core/modules/views/src/Tests/Handler/FilterStringTest.php
core/modules/views/src/Tests/Plugin/BlockDependenciesTest.php

That just enables the theme, it does not set it as default, unless I am missing something. We also need to ship a system.theme.yml config file which sets classy as default (With the same @todo).

+1 for the approach of enabling and using classy in the testing profile - will definitely be the sane way to sort this out whilst keeping existing test coverage.

I added system.theme.yml and a follow up issue to #2352949: Deprecate using Classy as the default theme for the 'testing' profile

* with new line

omg and patch

wow. spam much?

sorry, not very classy :(

Going to see if I can get less fails on this one. Thanks for bumping this along @Tim Bozeman :)

This probably won't be green (and either way it looks like we might want to update the following line in KernelTestBase), but this should get us further:

\Drupal::config('system.theme')->set('default', 'stark');

The only change here is moving system.theme.yml into config/install so no interdiff.

I changed KernelTestBase:206 from

 \Drupal::config('system.theme')->set('default', 'stark');

to

\Drupal::config('system.theme')->set('default', 'classy');

Many tests reference stark. I started here with FeedCacheTagsTest and worked up the class tree for that test.

This is just a continuation. I'll stop here because this is ultimately just a find-and-replace of stark and classy.

Wow, that's quite a lot of changes. Since this is only meant as a temporary measure, we're going to end up having to revert all those changes later on. So I think we should either

1. Change all those instances to use the default theme dynamically instead of hardcoding it

2. Reconsider whether this issue is worth it.

The worth it part is that we can't proceed with moving templates to Classy without this fix. Because many of the web tests check for classes in the outputted markup, moving the classes from core to Classy breaks those tests. Either we change all the tests, or try to get everything to use Classy. I'm also open to any other clever solutions, if anyone can think of any.

The temporariness is debatable. I have a feeling it won't be temporary, and 8 will release with it. (one person's opinion) But I don't think we've come up with any reasons why it's bad if it gets released this way.

Yeah it might be good to have this dynamically but Im not sure if its possible in reasonable time.

Yeah, lets see if this works o/

\o

I'll take a stab at removing the hard-codedness mentioned in #23.

This replaces hard-coded instances with a class variable.

Oops, missed a few.

This should fix the notices.

I think we need to first discuss whether we want to add this 'dynamic' thing there. I'm not sure if there's gonna even be way back anytime soon. Most of the tests need the classes that are not there anymore and what I looked there's a lot of them where its going to be hard to find any good alternative solutions without decreased test coverage.

It's still not 100% and it creates more complexity. Also all the test writers should be aware of it to use it later. I think its more clear for people if we are only using hard coded value of the theme. It's already confusing enough to use Classy there I think.

I'm going to unassign you jhedstrom. Thanks for your work! You can keep working on the issue while we've discussed this.

I think we need to first discuss whether we want to add this 'dynamic' thing there. I'm not sure if there's gonna even be way back anytime soon.

I don't think it's meant to be dynamic--it simply makes future changes simpler than this one. It will also make it easier to find (and fix) the tests that fail based on the assumption of a given theme (which led to this issue in the first place).

I'm not strongly in one camp or the other. I'm strongly planted in the "whatever works" camp. That said, two thoughts. One, is it really going to be that hard to change back, especially if we know what we changed? Two, does using the variable make it harder to work with if this stays in permanently?

The big picture here is that we still don't have a solid idea of whether we'll be able to make all the changes necessary to undo this change, so we should try to limit changes we could regret.

If there are tests that rely on classes existing, etc, then why should this not be a permanent change?

There is/was hope that those tests can be changed, as they're using the class check as somewhat of a convenience to verify the existence of the right content. However, there are a lot, and some would require completely rewriting the test.

The negative is that we will be, in part, testing the base theme instead of just whatever core functionality the tests were covering. But I think we crossed that bridge when we started moving the classes from preprocess to the templates. As soon as we did that, we are involving template files in the tests. The question just becomes where does the template file come from?

All of the self:: calls should be static:: in order to allow them to be overridden.

This changes selff::$theme to static::$theme. Sorry, no interdiff as I just replaced directly in the patch file rather than manually replacing.

Late in the Drupal 8 development cycle, we have moved core classes out of core and into Classy, therefore Classy should be used for testing. This seems like a clear, good, logical conclusion. It should not be viewed as temporary, but instead as the standard for Drupal 8.

If we want tests to be written differently, so that functionality of modules can be tested without using Classy, we write up the documentation and recommend the best practice and have module maintainers update their tests at the next major Drupal release. But for Drupal 8, use Classy for testing... and move on.

I recommend changing title of the thread to, "Use the Classy theme in the Testing profile".

The patch above lost a key bit of functionality (in changing the default theme). This is an interdiff from the working patch in #38.

This will need a well publicized change record.

I don't want to step on anyones toes but we had a discussion in the Twig meeting and I guess everyone at least on some level agreed there that we should implement on this issue the most simple possible solution. What I think we need to full fill that is just using the hard coded theme all over the place. I think @bradwade said that we shouldn't be against anyone or anyone's ideas but just moving this forward as fast as possible because we are blocking tens of issues with this.

I think its a good idea to have dynamic theme on the tests but the problem is that its not in the scope of this issue and because of that it will never get good enough. For this reason I would suggest us to create that in a follow-up. We can use the solution @jhedstrom has given here as a starting point there.

I have also pointed out my other concerns on doing that there #39.

One addition to my previous concerns is that its hard to fallback because of contribs might also rely on using classy as base for tests so we would break those contrib tests also.

So here I have a patch with the latest working solution with hard coded theme.

I think changing from a hard-coded string, to a static string variable is well within the scope of this for all the reasons previously stated. I think the word 'dynamic' was misused above. The change in #51 is no more dynamic than hard coding 'classy'. It simply removes all those hard-coded instances and makes them easier to change in the future (which will be desirable at some point as we move tests away from relying on themes/classes, making future patches simpler).

Changing title as suggested by bradwade in #48.

+1 for hard-coded for now. Part of the issue raised during the call was added complexity with tests potentially having the check the variable, or not having an understanding of which theme was actually being used and confusing people. The usefulness of being able to more easily change it is mitigated by the fact that we don't think this is changing before release.

We really would like to work out some additional cleverness in how the theme is handled during testing, but this isn't the time to do it. I want to RTBC something now and get this in, as it is blocking too many things.

I don't think it makes moving back that much easier because we still have to check all the instances whether people have used that variable or not. I personally wouldn't expect something like that to exist. I also think that is absurd to base any arguments on moving back because its not happening. I think it's also unnecessary complexity (people don't see immediately which theme their using). Good thing is that if you want to use Stark somewhere you could set this variable to Stark and also change the theme but I don't think it's argument here because its not in the scope of this issue.

Anyways I'm not gonna argue here forever. If anyone else has opinion please express it here, I'm done. I just want to get this issue from blocking those issues.

Here's patch which should be more bot friendly.

I completely agree with #60;)

+++ b/core/themes/classy/classy.info.yml
@@ -4,3 +4,7 @@ description: 'A base theme with sensible default CSS classes added. Learn how to
diff --git a/core/themes/classy/logo.png b/core/themes/classy/logo.png

diff --git a/core/themes/classy/logo.png b/core/themes/classy/logo.png
new file mode 100644

new file mode 100644
index 0000000..32332cf

index 0000000..32332cf
--- /dev/null

--- /dev/null
+++ b/core/themes/classy/logo.png

+++ b/core/themes/classy/logo.png
+++ b/core/themes/classy/logo.png
@@ -0,0 +1,8 @@

@@ -0,0 +1,8 @@
+�PNG

Did this sneak in by accident?

No, there's tests requiring it

There should be also empty layout.css file which is required by some tests. (not the file but loading that file)

Adding beta phase evaluation.

@lauriii, can you post an interdiff, please?

#64 is RTBC. It will unblock a tremendous amount of work in the front end, and we can follow up in a new issue, as mentioned above, to address the hard-coded nature of the tests.

Just some notes:
- Even though we might not get it done for 8.0.0 it must certainly be the end-goal to have all core tests have as few dependencies as possible and that includes any Classy markup. Reasons:
A) This improves test isolation
B) Improves performance of the test runs
C) Since Classy is a completely optional theme and the tests of modules should be testing module functionality, there should be absolutely no reason that the modules rely on Classy markup and thus the tests shouldn't either. Separation of functionality and appearance, anyone?

- It is highly preferable to use a dynamic solution to avoid future refactorings. Not only will this make going back from Classy to Stark slightly easier, more importantly it will completely eliminate the issues we are facing here if we ever decide to rename Classy or Stark of whatever in the future, or if we have a new default theme in the future. This is just a matter of decoupling systems. A test should not have any knowledge about what we decided to call our default theme, whether its Classy, Stark or whatever. I shouldn't have to know in order to write a test, even one that interacts with themes.

So from my point of view #51 is RTBC not #64 but I'll let you guys procede here. Just wanted to point the above out, because I don't think these points were considered above.

@tstoeckler: I understand your point of you but those things are not in the target of this issue. This issue is created to unblock all the Banana Phase 2 issues by using classy as a default theme on the tests. We had discussion about this issue on Twig meeting and we all agreed that we have to get this issue away from blocking all the Banana Phase 2 issues. Because of that we wanted to find a solution with least resistance. We can do all the improvements on a follow-up and have the discussion about that properly there.

Sure. I find that process a bit strange because it seems the work was already done here (#51) but I don't want to hold up the awesome Banana effort in which you guys have put so much effort in any way. That's why I left it at RTBC. :-)

I agree that we keep the original scope and create an issue about making the testing theme changeable as there might be couple of thing to consider like how to write tests when your XPaths might be different depending on the theme used...

I'm just adding a comment to the empty layout file that it is used for testing.

I think it would be helpful to @see the test that needs this file.

https://www.drupal.org/node/1354#see

also the issue summary needs updated.

if the tasks in the remaining tasks list are done, update the line in the summary pointing out which comment number did it.
if the tasks are not done.. do them. :)

looks like there is a change record draft, but the needs change record tag is still on the issue, does the change record need review?

Removing the change record tag

Change record needs review.

@lauriii missed the second test. Adding this now.

Adding the second test.

Looks good, moving this back to RTBC. Just a small change since #67.

Added followup for adding test coverage to Classy #2380401: Add test coverage to Classy theme

Change record looks good to me. Updating issue summary and marking "Review the change record" as complete.

Committed bbb4de9 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

We caused a problem with Bartik that obviously no one noticed. If we don't fix it quickly, we need to roll this back. I added another issue. #2380605: Bartik layout broken

Please @alexpott don't commit front-end patches without screenshots.

Should be all better now, whew.

I created #2381305: Allow testing theme to be more easily changed as follow-up.

I apologize for my comment in #90. I was told my comment was rude so I am sorry about it.
Thank you everyone for fixing this and follow up issue.

I updated the CR to address the fact that this breaks any test that assumed stark was installed: https://www.drupal.org/node/2380181/revisions/view/7863059/7890643