Problem/Motivation

Whenever you are missing config schema, integration tests fail. This happens a lot right now with many contrib modules missing schema and also bugs in core such as #2392057: Config schema fails to expand dynamic top-level types

Proposed resolution

Allow the test class to pass in the exceptions so we can exclude individual config objects without having to set strictSchemaCheck = FALSE while patches get committed etc.

Remaining tasks

Review

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Comments

benjy created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, config-schema-exceptions.patch, failed testing.

The last submitted patch, config-schema-exceptions.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
StatusFileSize
new4.36 KB
new902 bytes

Lets try this.

benjy’s picture

Issue tags: +rc eligible

Tagging this rc eligible given it's test only changes.

jibran’s picture

Category: Feature request » Task

Given that we are only making the existing functionality configurable by moving it to base class that's why I think it's a task. This is good addition and very helpful for contrib.
Patch looks good as is but we also use static:$module pattern in TestBase and then merge all the arrays in setUp method. Do you think we should also do that here as well so that no test can overwrite the existing array. Other then that I think this change needs change record.

benjy’s picture

StatusFileSize
new5.03 KB
new2.6 KB

OK, i've refactored to merge all test exceptions the same way modules works.

Do we normal write change records for changes only in tests?

jibran’s picture

Status: Needs review » Reviewed & tested by the community

I think it is worth adding change record or at least update All TestBase derived tests now enforce strict configuration schema adherence by default. Patch looks good now.

benjy’s picture

OK, i'll update the existing change record with an example of this, but not until after it's committed.

sam152’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php
@@ -44,13 +44,23 @@ class ConfigSchemaChecker implements EventSubscriberInterface {
+  protected $exceptions = array();
...
+    $this->exceptions = $exceptions;

+++ b/core/modules/simpletest/src/TestBase.php
@@ -1585,4 +1601,23 @@ public function getTempFilesDirectory() {
+      if (property_exists($class, 'configSchemaExceptions')) {

Different property names are used here.

sam152’s picture

Status: Needs work » Reviewed & tested by the community

Nevermind, configSchemaExceptions is used on the test base and exceptions on the event.

Setting back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
StatusFileSize
new5.01 KB
new5.1 KB

I started to rename some things on commit and make everything more consistent. I got too large so uploading a new patch. I think that using the word exceptions is confusing when the ConfigSchemaChecker throws exceptions. I've changed it to be exclusions and I've made all the doc blocks consistent.

I agree this change is rc eligible.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, thanks.

jibran’s picture

+1 to RTBC.

effulgentsia’s picture

Looks good. Ticking credit box for @jibran for reviews.

effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Pushed to 8.0.x.

  • effulgentsia committed 74a5023 on 8.0.x
    Issue #2580389 by benjy, alexpott, jibran: Allow test classes to specify...

Status: Fixed » Closed (fixed)

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