Problem/Motivation

WebTestBase does config schema checking for the site under test. BrowserTestBase should have this too. Per Alex Pott in #2547447-14: Stop creating services.yml by default and -15.

Proposed resolution

Let BrowserTestBase also check config schema for the installed config.

Remaining tasks

  1. Add tests.
  2. Get patch green.
  3. Review.
  4. Commit.

User interface changes

None.

API changes

None. Soft behavior change: config schemas will be checked also for BrowserTestBase tests. Which means broken config will be caught in those tests too.

Data model changes

None.

CommentFileSizeAuthor
#38 2553733-38.patch8.25 KBalexpott
#38 34-38-interdiff.txt1015 bytesalexpott
#34 2553733-34.patch7.99 KBalexpott
#34 2553733-34.test-only.patch5.2 KBalexpott
#32 2553733-32.patch4.05 KBalexpott
#32 28-32-interdiff.txt1.21 KBalexpott
#28 2553733-28.patch4.67 KBalexpott
#28 26-28-interdiff.txt679 bytesalexpott
#26 2553733-26.patch5.33 KBalexpott
#26 2553733-26.test-only.patch1.26 KBalexpott
#25 2553733-25.patch4.07 KBalexpott
#23 2553733-23.patch3.67 KBdawehner
#18 interdiff.txt1.39 KBdawehner
#18 2553733-18.patch3.67 KBdawehner
#16 interdiff.txt1.6 KBdawehner
#16 2553733-16.patch3.47 KBdawehner
#2 2553733-2.patch2.25 KBwim leers

Comments

alexpott created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 2553733-2.patch2.25 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2553733-2.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented

Is that some logic we can move into a trait and share so?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Good idea!

(If BrowserTestBase extended TestBase we could put it there. But since it doesn't, a trait is the only option.)

dawehner’s picture

dawehner
Primary language German
commented

And the better one ... IMHO you also maybe want to validate config on kernel tests?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Can we include this fix in next reroll https://github.com/jibran/drupal/commit/31dd5cc5a01ed863a36299508f1098d9...?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +rc target, +Needs committer feedback

I suspect we should still do this, because it'll catch problems that'd otherwise go unnoticed.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

(And adding it after RC would mean breaking contrib projects.)

dawehner’s picture

dawehner
Primary language German
commented

Yeah we totally should.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Related issues: +#2503547: Contrib can't run Functional tests

BTB for contrib is kinda a broken #2503547: Contrib can't run Functional tests so I think it is ok after RC as well.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Interesting, thanks for letting us know!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes

Added beta evaluation explaining impact vs. disruption wrt rc target tag; better justifying #8's added tags.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -rc target, -Needs committer feedback +rc target triage

We have a tag for that now. :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: -rc target triage +rc eligible

As a test infrastructure change this is actually "rc eligible"

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new 2553733-16.patch3.47 KB
new interdiff.txt1.6 KB

Let's fix it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work 
+++ b/core/modules/simpletest/src/BrowserTestBase.php
@@ -906,9 +929,13 @@ public function installDrupal() {
+    $settings = [];
+    $settings['settings']['file_private_path'] = (object) [
+      'value' => $this->privateFilesDirectory,
+      'required' => TRUE,
+    ];

+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -1075,7 +1075,7 @@ public function __get($name) {
         case 'private_files_directory':
-          return $this->container->get('config.factory')->get('system.file')->get('path.private');
+          return Settings::get('file_private_path');

I was going to say that this is a separate issue - but it is not! This is why config schema checking is a good thing(tm)

The one in KernelTestBase test implies we don't have schema checking turned on there - I think we should have - but that is a separate issue.

+++ b/core/modules/simpletest/src/BrowserTestBase.php
@@ -862,9 +871,23 @@ public function installDrupal() {
+      $yaml = new \Symfony\Component\Yaml\Yaml();

I guess we should have a use statement instead of a fully qualified class.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new 2553733-18.patch3.67 KB
new interdiff.txt1.39 KB

The one in KernelTestBase test implies we don't have schema checking turned on there

I'm like 99.5% sure that at least the new kernel test has schema checking turned on by default. For contrib modules I used kernel tests to ensure that schema is actually there.

I guess we should have a use statement instead of a fully qualified class.

Let's go with the variant in \Component, in case we ever will make it possible to use the PECL implementation.

Status: Needs review » Needs work

The last submitted patch, 18: 2553733-18.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Version: 8.0.x-dev » 8.2.x-dev

.

The last submitted patch, 18: 2553733-18.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review

.

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2553733-23.patch3.67 KB

Reapply to 8.2.x

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Priority: Normal » Critical

Oh wow I'd forgotten about this. This is now a critical regression because we have many more BrowserTestBase tests. :(

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: +Needs tests
StatusFileSize
new 2553733-25.patch4.07 KB

Rerolled

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2553733-26.test-only.patch1.26 KB
new 2553733-26.patch5.33 KB

Here's some tests.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -1104,7 +1104,7 @@ public function __get($name) {
           return Settings::get('file_public_path', \Drupal::service('site.path') . '/files');
 
         case 'private_files_directory':
-          return $this->container->get('config.factory')->get('system.file')->get('path.private');
+          return Settings::get('file_private_path');
 
         case 'temp_files_directory':

I guess this should have its own dedicated issue?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 26-28-interdiff.txt679 bytes
new 2553733-28.patch4.67 KB

@dawehner agreed...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Create #2779459: KernelTestBase thinks private file setting is in config for the KernelTestBase issue.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

I think this patch totally solves the problems we have, adds test coverage. What else would one want.

The last submitted patch, 26: 2553733-26.test-only.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 28-32-interdiff.txt1.21 KB
new 2553733-32.patch4.05 KB

Minor fix to a comment placement. And removal of unnecessary change.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Hmmm... actually I think I should port \Drupal\config\Tests\SchemaConfigListenerWebTest to BTB.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 2553733-34.test-only.patch5.2 KB
new 2553733-34.patch7.99 KB

So instead of \Drupal\config\Tests\SchemaConfigListenerWebTest I refactored \Drupal\KernelTests\Core\Config\SchemaConfigListenerTest to use a trait and used that.

The last submitted patch, 34: 2553733-34.test-only.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Works for me.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented 
+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -990,6 +1015,17 @@ public function installDrupal() {
+        'class' => 'Drupal\Core\Config\Testing\ConfigSchemaChecker',

Let's use class constant here.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 34-38-interdiff.txt1015 bytes
new 2553733-38.patch8.25 KB

Sure why not

catch’s picture

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

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

  • catch committed 937b9f2 on 8.3.x 
    Issue #2553733 by alexpott, dawehner, Wim Leers, jibran: BrowserTestBase...

  • catch committed d57ab31 on 8.2.x 
    Issue #2553733 by alexpott, dawehner, Wim Leers, jibran: BrowserTestBase...

Status: Fixed » Closed (fixed)

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