Follow-up to #2293105: MigrateActionConfigSchemaTest duplicates all its code from SchemaCheckTestTrait

Problem/Motivation

#2293105: MigrateActionConfigSchemaTest duplicates all its code from SchemaCheckTestTrait adds config schema test for MigrateAction. But we have a list of config tests which don't have coverage.

Proposed resolution

Use SchemaCheckTestTrait in remaining config tests and fix any fails.

Remaining tasks

Patch.

User interface changes

N/A

API changes

N/A

Comments

vijaycs85’s picture

vijaycs85’s picture

Status: Postponed » Needs review
FileSize
20.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,805 pass(es), 10 fail(s), and 0 exception(s). View
vijaycs85’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 2: 2293419-cleanup-migrate-config-schema-2.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
24.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,813 pass(es), 4 fail(s), and 0 exception(s). View
3.29 KB

Not fixed:
Looks like we are trying to migrate below variables as config, but they don't exist as config or moved to some other place.

  1. aggregator_category_selector => aggregator.settings:source.category_selector (Related: #2127725: Remove category handling from aggregator)
  2. field_language_fallback => field.settings:language_fallback

Fixed:

  1. statistics.settings:access_log.enable: Missing schema - typo, should be statistics.settings:access_log.enabled
  2. locale.settings:cache_string: Missing schema - typo, should be locale.settings:cache_strings
  3. update.settings:notification.mails: Non-scalar value but not defined as an array (such as mapping or sequence) - typo, should be update.settings:notification.emails

This patch would produce 4 fails for the 2 items in 'not fixed' (as they are running twice).

Status: Needs review » Needs work

The last submitted patch, 5: 2293419-add-config-schema-test-5.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +sprint

Wow, good job! #2293063: Config schema mapping parsing is inconsistently forgiving, does not conform to the interface also found several issues with migration tests, but none of them are overlapping, so looks like we are fine doing these separately :)

Gábor Hojtsy’s picture

Can we also get rid of MigrateActionConfigSchemaTest in favor of just a method in an existing test like the rest here? Now that #2293105: MigrateActionConfigSchemaTest duplicates all its code from SchemaCheckTestTrait landed :)

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
27.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,814 pass(es), 4 fail(s), and 0 exception(s). View
2.86 KB

yep, we can remove it part of this issue.

Status: Needs review » Needs work

The last submitted patch, 9: 2293419-add-config-schema-test-9.patch, failed testing.

vijaycs85’s picture

Created 2 new issues #2294341: Migrate aggregator_category_selector variable to D8 properly and #2294345: Migrate field_language_fallback variable to D8 properly to track those 2 fails... Is it Ok to remove those config to move forward with this issue?

vijaycs85’s picture

Issue tags: +LONDON_2014_JUNE
benjy’s picture

Since neither of them are used in D8 anymore, from what I can tell, lets remove them and we can figure out what to do in the relevant issues.

Gábor Hojtsy’s picture

I agree its fine to remove them if those are not used in D8 anymore especially if you have issues to figure them out.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
31.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,184 pass(es), 1 fail(s), and 2 exception(s). View
6.85 KB

Ok, removing unused variables and associated migration code.

Status: Needs review » Needs work

The last submitted patch, 15: 2293419-add-config-schema-test-15.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
32.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,386 pass(es), 1 fail(s), and 0 exception(s). View
701 bytes

removing test leftover...

Status: Needs review » Needs work

The last submitted patch, 17: 2293419-add-config-schema-test-17.patch, failed testing.

andypost’s picture

Working on #2292821: Use widget for comment subject field I found that migrate schema for comment migrations is inconsistent

+++ b/core/modules/migrate/config/schema/migrate.source.schema.yml
@@ -0,0 +1,388 @@
+migrate.source.d6_comment:
...
+migrate.source.d6_comment:

how does duplicated keys works?

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
32.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,833 pass(es). View
734 bytes

another test cleanup...

how does duplicated keys works?

- the later one override and it is a mistake.

Gábor Hojtsy’s picture

Title: Add config schema test to all configuration test in migration » Add config schema test to all configuration test in migration, fix bugs
Category: Task » Bug report
Status: Needs review » Reviewed & tested by the community

I think this looks great for the scope of making migrates tested and indeed found several bugs in migrations :) Good stuff.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Really nice work!

+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateActionConfigsTest.php
@@ -55,4 +58,12 @@ public function testActionSettings() {
   }
 
+  /**
+   * Tests configuration schema.
+   */
+  public function testConfigSchema() {
+    $config_data = \Drupal::config('action.settings')->get();
+    $this->assertConfigSchema(\Drupal::service('config.typed'), 'action.settings', $config_data);
+  }
+
 }

I don't think we should be adding a separate test method to all the migrate tests. This should be combined with testActionSettings since that Tests migration of action variables to action.settings.yml. and so does this. This will save 2 additional test set ups since these tests are run twice.

This pattern is repeated a lot throughout the patch - all instances need fixing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
30.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,876 pass(es). View
17.36 KB

Thanks for your feedback @alexpott. Here is an update for #21.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks even better.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed acb96c0 and pushed to 8.x. Thanks!

  • alexpott committed acb96c0 on 8.x
    Issue #2293419 by vijaycs85: Fixed Add config schema test to all...
Gábor Hojtsy’s picture

Status: Fixed » Closed (fixed)

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

chx’s picture

Status: Closed (fixed) » Active

The migration system uses the config API. Why doesn't the config API enforce the schema? Are we going to need to test every module that uses the config API?? This shouldn't be necessary and should be rolled back.

Gábor Hojtsy’s picture

Config save checks the schema now. It did not check it yet at the time of this issue because our schemas were far from complete. I proposed removing these explicit tests at the time, but @alexpott said they do no harm.

alexpott’s picture

Status: Active » Fixed

As @Gábor Hojtsy pointed out we do have automated testing of config schema now and it now test migrate's use of config too so yes it can be removed I stand by the tests doing no harm. @chx is you want to remove the tests - fair enough - but new issue please.

Status: Fixed » Closed (fixed)

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