Add automated test coverage for Claro

Re #3:

Well, we didn't inherit anything like this from the Seven. Actually, these configurations were asked by AND added by you :) here. And I was the one who merged the PR :)
The thing that provides these block configs for the Seven theme is the Standard install profile.

Here's what I'd do with the config. The problem with the shortcut third party setting is there is literally no way for a theme to put that there dynamically. So let's enable shortcut in the test and comment to that affect. It's more important that the config in claro.settings is valid rather than the schema complete when shortcut is missing. We need to open a follow-up issue about third party settings in simple config. It's really messy.

For blocks we can install the block during the test - as done in the patch.

What's neat here is that we show that Claro is triggering a deprcated code path yay!

This fixes the deprecation errors by moving the prerender callbacks to a class.

So the build is failing cause of PHPCS for standards...

@lauriii mentioned a need for a bit of FunctionalJavascript coverage, so here's a start with tests that extend existing FunctionalJavascript tests. I'm not even sure if extending tests in this way is permissible, but I suppose that's what reviews are for.

If this approach is fine, it would be good to add some views tests, but many might fail until #3067386: AJAX rebuild of Views UI form re-renders parts of the form without using Form API, so additional hooks are needed in lieu of hook_form_view_edit_form_alter() lands.

I think extending tests is okay - it does mean when the parents test changes Claro gets the coverage immediately - which is good. But it also could mean testing things we don't need to. I think a general rule is if we need to change the test beyond just emptying one out then we probably shouldn't be extending.

+++ b/tests/src/FunctionalJavascript/ClaroEntityDisplayTest.php
@@ -0,0 +1,116 @@
+/**
+ * Runs EntityDisplayTest in Claro.
+ *
+ * @group claro
+ *
+ * @see \Drupal\Tests\field_ui\FunctionalJavascript\EntityDisplayTest.
+ */
+class ClaroEntityDisplayTest extends EntityDisplayTest {

This overrides ever test in the parent... I think I'd copy the parents setUp and $modules and extend WebDriverTestBase so we don't couple the tests.

Added test coverage that proves the Claro theme can be uninstalled, despite being experimental.

+++ b/tests/src/FunctionalJavascript/ClaroBlockFilterTest.php
@@ -0,0 +1,36 @@
+  protected function setUp() {
+    parent::setUp();
+    $this->container->get('theme_installer')->install(['claro']);
+    $this->config('system.theme')->set('default', 'claro')->save();
+  }
+

+++ b/tests/src/FunctionalJavascript/ClaroEntityDisplayTest.php
@@ -0,0 +1,116 @@
+  protected function setUp() {
+    parent::setUp();
+    $this->container->get('theme_installer')->install(['claro']);
+    $this->config('system.theme')->set('default', 'claro')->save();
+  }

This is a fun coincidence!

This is subclassing existing tests and changing the default theme. That happens to be exactly what #2352949: Deprecate using Classy as the default theme for the 'testing' profile simplified 🥳 So we can change this many times in the patch, like so:

diff --git a/tests/src/FunctionalJavascript/ClaroBlockFilterTest.php b/tests/src/FunctionalJavascript/ClaroBlockFilterTest.php
index da7b3ac..42732e9 100644
--- a/tests/src/FunctionalJavascript/ClaroBlockFilterTest.php
+++ b/tests/src/FunctionalJavascript/ClaroBlockFilterTest.php
@@ -27,10 +27,6 @@ class ClaroBlockFilterTest extends BlockFilterTest {
   /**
    * {@inheritdoc}
    */
-  protected function setUp() {
-    parent::setUp();
-    $this->container->get('theme_installer')->install(['claro']);
-    $this->config('system.theme')->set('default', 'claro')->save();
-  }
+  protected $defaultTheme = 'claro';
 
 }

BUT! This won't work if Claro also needs to work on 8.7. Fortunately, Claro 8.x-2.x is targeting 8.8 only! 😎 So we can do this :)

Forgot one spot.

+++ b/tests/src/FunctionalJavascript/ClaroMenuUiJavascriptTest.php
@@ -0,0 +1,37 @@
+  /**
+   * {@inheritdoc}
+   */
+  protected $defaultTheme = 'claro';

<3 :D

@alexpott just pointed out in Slack that #13 failed because of a bug in #2352949: Deprecate using Classy as the default theme for the 'testing' profile:

    $this->installDefaultThemeFromClassProperty($container);
    $this->installModulesFromClassProperty($container);

are the wrong way around 🤦‍♂️

Until that is fixed, we should revert back to #12.

I was going to open a new core issue for @alexpott's suggestion but … it also doesn't work. I made this change:

diff --git a/core/tests/Drupal/Tests/BrowserTestBase.php b/core/tests/Drupal/Tests/BrowserTestBase.php
index 759b6033ea..5be3750de7 100644
--- a/core/tests/Drupal/Tests/BrowserTestBase.php
+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -566,8 +566,8 @@ public function installDrupal() {
     $this->initSettings();
     $container = $this->initKernel(\Drupal::request());
     $this->initConfig($container);
-    $this->installDefaultThemeFromClassProperty($container);
     $this->installModulesFromClassProperty($container);
+    $this->installDefaultThemeFromClassProperty($container);
     $this->rebuildAll();
   }
 

but Claro installation still fails the same way:

Testing started at 11:34 ...
/usr/local/bin/php /private/var/folders/4l/5sndwsz50tqgxjh2525n10br0000gp/T/ide-phpunit.php --configuration /Users/wim.leers/Work/d8/core/phpunit.xml Drupal\Tests\claro\Functional\ClaroTest /Users/wim.leers/Work/d8/themes/claro/tests/src/Functional/ClaroTest.php
#!/usr/bin/env php
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\claro\Functional\ClaroTest
EE                                                                  2 / 2 (100%)

Time: 7.67 seconds, Memory: 6.00MB

There were 2 errors:

1) Drupal\Tests\claro\Functional\ClaroTest::testRegressionMissingElementsCss
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for claro.settings with the following errors: claro.settings:third_party_settings.shortcut missing schema

/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:95
/Users/wim.leers/Work/d8/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:111
/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Config/Config.php:231
/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Config/ConfigInstaller.php:378
/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Config/ConfigInstaller.php:137
/Users/wim.leers/Work/d8/core/lib/Drupal/Core/ProxyClass/Config/ConfigInstaller.php:75
/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Extension/ThemeInstaller.php:188
/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:435
/Users/wim.leers/Work/d8/core/tests/Drupal/Tests/BrowserTestBase.php:570
/Users/wim.leers/Work/d8/core/tests/Drupal/Tests/BrowserTestBase.php:398

2) Drupal\Tests\claro\Functional\ClaroTest::testIsUninstallable
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for claro.settings with the following errors: claro.settings:third_party_settings.shortcut missing schema

/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:95
/Users/wim.leers/Work/d8/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:111
/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Config/Config.php:231
/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Config/ConfigInstaller.php:378
/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Config/ConfigInstaller.php:137
/Users/wim.leers/Work/d8/core/lib/Drupal/Core/ProxyClass/Config/ConfigInstaller.php:75
/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Extension/ThemeInstaller.php:188
/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:435
/Users/wim.leers/Work/d8/core/tests/Drupal/Tests/BrowserTestBase.php:570
/Users/wim.leers/Work/d8/core/tests/Drupal/Tests/BrowserTestBase.php:398

Reverting back to #12. The #13 additional interdiff, its failure, plus #16 and #17 are out of scope for this issue anyway.

(And renamed -12.patch to -18.patch.)

I agree with #18. Here's the core issue to fix this #3087036: third_party_settings in THEME.settings.yml cannot be dependency managed

Two patches for two different things and I acknowledge that this is probably confusing

3086435---extra-tests-written-off-9.patch is additional changes I was making working off of #9. The patches that came after #9 appeared while I was working on this and I haven't had the chance to integrate the two. There is a failure in VerticalTabsTest happening (per my burner issue) on testbot that isn't happening locally.

3086435-18-with-FunctionalJavascript-enabled.patch is just the patch from #18 but with javascript testing enabled in drupalci.yml The switch to protected $defaultTheme = 'claro'; from enabling the theme in setup seems to bring back

Drupal\Core\Config\Schema\SchemaIncompleteException : Schema errors for claro.settings with the following errors: claro.settings:third_party_settings.shortcut missing schema

Which I already confirmed in another burner issue test if this is being read before drupalci has finished running.

So

• The additions from 3086435---extra-tests-written-off-9.patch need to be integrated with changes that happened in later patches
• The testbot-only fails in VerticalTabsTest (last i knew) need to be fixed
• The shortcut schema errors likely due to protected $defaultTheme = 'claro'; need addressing.

The shortcut schema errors likely due to protected $defaultTheme = 'claro'; need addressing.

These should be addressed by enabling the theme in set up like the Functional test.

3086435-18-with-FunctionalJavascript-enabled.patch is just the patch from #18 but with javascript testing enabled in drupalci.yml The switch to protected $defaultTheme = 'claro'; from enabling the theme in setup seems to bring back

Yeah just don't do that. 🙂 Just like #18 doesn't do that, and core committer and configuration system maintainer @alexpott already confirmed this approach in #19 :) And … I see that @alexpott just commented to the same effect in #23 🤓👍

There is a failure in VerticalTabsTest happening (per my burner issue) on testbot that isn't happening locally.

I first though it was

$tab_one = $assert_session->elementExists('css', '.vertical-tabs__menu-link:contains("First group element")');

Because :contains selectors technically aren't supported in CSS. But apparently they are in Chrome? :O Or our testing infrastructure maps this to XPath selectors automatically? 🤯🤷‍♂️

Either way, that was a dead end! The real answer is much simpler: Claro tests are run without yarn build:css. So requests to /themes/claro/css/dist/* result in 404s. And since this test is doing highly CSS-dependent things … the test fails on testbot, but not locally. You can make it fail locally by removing css/dist and not running yarn build:css.

3086435-18-with-FunctionalJavascript-enabled.patch is just the patch from #18 but with javascript testing enabled in drupalci.yml The switch to protected $defaultTheme = 'claro'; from enabling the theme in setup seems to bring back

To address this: including the drupalci.yml changes in that patch, but reverting #12's interdiff.

I don't think #25 can be fixed in Claro contrib, because Claro contrib does not commit the results of yarn build:css to its git repository.

But Drupal core does.

So #25's VerticalTabsTest could be committed to Drupal core when Claro is added to core. But it cannot be added to Claro in contrib.

I think it's laudable that Vertical Tabs behavior is being tested here, but … the intended scope of this issue was somewhat more minimal:

I think we should add at least some minimal test coverage to the must-have list. Even if it's just a single test function similar to what we have for BartikTest and ClassyTest. Or maybe a subclass of InstallUninstallTest that executes all of the same stuff but with Claro as the Admin theme.

The latest patch already gives far more assurances than that minimal test coverage. So I think this is adequate as-is.

I agree with #25. The test coverage added here is a great smoke test and we can improve it as we go. We've got evidence this will help us keep Claro up-to-date and catch errors so +1 to getting this done.

For reasoning similar to #25 (and approved by @alexpott in #28), I'm dropping ClaroManageDisplayTest too, because that too suffers from config-related challenges.

Yay, green! 🥳

#32: The idea was that this should be removed when Claro becomes stable. That's the issue closest to "Claro is stable" I could find.