Improve \Drupal\KernelTests\Config\DefaultConfigTest to install all optional configuration

Some work was done on the views max-age part of this here #3038408: Update config/*/views.view.*.yml tables with correct dynamic cache max-age, but that changes more instances of that then the patch here, like in views.view.media.yml, so what might be the reason those are not caught and the other here are?

Marking #2989007: Various shipped views have an incorrect cache max-age as a duplicate, as well. I had found that via Config Overlay but never thought about DefaultConfigTest so it's awesome to actually have test coverage here, nice work!

There are two things that the current patch does not include that I found there: (It's not in the patch there, though, yet, search here for 2989007 (that issue's ID) for the nitty gritty details.)

1. views.view.moderated_content is missing a dependency on the workflows.workflow.editorial configuration
2. tour.tour.umami-front is missing a dependency on demo_umami_tour

Not sure of the implications for DefaultConfigTest for that, i.e. I'm totally fine with the patch going in as is and handling those cases (ideally with test coverage) in a separate issue, but wanted to bring it up.

@alexpott it's catching more but still not everything, if I export views.view.media.yml locally on a clean Umami install, it is not identical to the default config, caching: max-age is -1.

But it is catching the fact that the config dependency is missing in HEAD in the same view, so its not like this config is not loaded during the test. Did a little digging but can't really see why this would be.

@alexpott thanks for addressing all the feedback, with #10 in mind I think this is ready. The other scenario's are out of scope for this change.

Good addition, makes sense.

+    if (method_exists($this, 'assertTrue')) {
+      $this->assertTrue(TRUE, "$config_name has no differences");
+    }

just want to say eww... Not like stop this patch bad because looking at it that whole method is kinda of goofy so doing anything else would mostly require rewriting it. assertTrue(True is just a huge red flag you are doing something wrong so we should probably have a follow up.

Agree w/ #14 about AssertConfigTrait eww. :-)

+++ b/core/tests/Drupal/KernelTests/AssertConfigTrait.php
@@ -85,6 +85,9 @@ protected function assertConfigDiff(Diff $result, $config_name, array $skipped_c
+    if (method_exists($this, 'assertTrue')) {
+      $this->assertTrue(TRUE, "$config_name has no differences");
+    }

Replace with:

use AssertLegacyTrait;

[..]

      $this->pass("$config_name has no differences");

Because in this case, we need AssertLegacyTrait so we're compatible with WTB, and so that we find this later when we remove it.

Re @alexpott in #10:

1. Thanks for taking the time to look into that. I will investigate what's going on there if I have some time.
2. We already have DemoUmamiProfileTest::testConfig() that tests the profile's configuration. In this case this is configuration that is inside of profile-specific module, though, which we do not test. But instead of expanding the test to cover that, I think we should remove that module entirely (and fold the configuration into the profile itself), so opened #3060602: demo_umami_tour should not be a separate module. If my theory is correct the patch there (that I have yet to upload) will make DemoUmamiProfileTest::testConfig() fail without the change to the configuration. I agree that it would be nice to have proper testing infrastructure in the form of base classes for modules and profiles. I had opened #2939753: Extract DemoUmamiProfileTest::testConfig() into a ProfileTestBase for exactly that purpose because it seemed like a nice first step, but it never got done unfortunately.

Again, great work on this! I will greatly enjoy removing a bunch of cruft from Config Overlay's test suite.

Looked into the workflow thing a bit, it's pretty weird and I don't have the actual cause yet, but I found/opened: #3060633: Check that views do not get created with broken handlers

AssertConfigTrait is also used by Drupal\system\Tests\Installer\ConfigAfterInstallerTestBase which is a deprecated simpletest test base, so saying we should add an assertion would amount to a nit.

But it's also used by Drupal\FunctionalTests\Installer\MinimalInstallerTest which won't have an assertion other than the functional test framework. (When it says 'minimal,' it means it.) That's out of scope here (DefaultConfigTest), and also out of scope in #3059332: Mark kernel tests that perform no assertions as risky (not a kernel test). That leaves the question of whether we can refactor assertions out of functional tests as we have in #3059332: Mark kernel tests that perform no assertions as risky for kernel tests, and I'd say that's not really feasible so we don't need a follow-up. And thus it doesn't matter if we change MinimalInstallerTest here.

RTBC other than that, especially after #13.

Yup, looks good.

[...] which shows how much this fix is needed

Agreed!

Also, since I'm already here: Shameless plug for #3060602: demo_umami_tour should not be a separate module which is green and a nice little related fix.

just faced this issue (#3062291: Config entity changes as part of install) when trying to write an upgrade test for #2909435: Update block library page to adapt publishable block content implementation. Also running test-only patch of #22 at #1970534-171: Patch testing issue to make sure failures at HEAD.

Committed 3263c8c and pushed to 8.8.x. Thanks!

Thanks (again)! Great to see this land, it allowed me to remove a lot of cruft.