#1653026: [META] Use properly typed values in module configuration seems to have broken deployment. #2105771: block.block.bartik.breadcrumbs and block.block.seven.breadcrumbs always show as need importing is one bug, there are lots of others out there we don't know about yet.

Because the process of setting up a dev/live deployment scenario is extremely time-consuming and error-prone to do manually, it's not shocking to me that people don't do it often when testing CMI patches. It is, however, the entire point of that feature, so we should make sure that it's always working even with various refactorings we do.

So we need a test that does (at least) something roughly like this:

- Set up prod
- Set up dev based on prod
- Change site slogan on dev
- Export config from dev
- Import config on prod
- Verify that site slogan and only site slogan appears on the synchronize list
- Import the changes on prod
- Verify that prod's synchronize list is now empty.
- Verify site slogan changed on prod
- Also do the same with something fancier, like make a content type with a field it on dev

...Because those are what I tried to do in front of an audience of 70 people or so today at PNWDS and it failed. Hard. :)

chx says the following could work for writing the test (there's some trickiness because test methods aren't guaranteed to run in any particular order, and order matters here):

1) Add a sort($class_methods) to TestBase::run based on a property
2) Write a test method that exports
3) At the end of the test do $this->export = file_get_contents('exported_tarball'); (to hold the contents between testruns so they dont' get deleted)
4) write a test method which is alphabetically later which imports (testExport, testImport will actually do)

Each testX method automatically spins up a new Drupal site, so the dev/prod would easily be covered.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Status: Active » Needs review
FileSize
2.96 KB

The test is here, the config import fails with
Attempt to save a configuration entity <em class="placeholder">user.register</em> with UUID <em class="placeholder">c1d211c4-7b4a-49e8-b900-0f69eab900f1</em> when this entity already exists with UUID <em class="placeholder">cd6e2b56-b951-4166-b435-53168378ea84</em> (I have dumped the exception message from ConfigSync::submitForm)

Status: Needs review » Needs work

The last submitted patch, 2106171_1.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
chx’s picture

FileSize
40.52 KB

This passes. Includes the patch from #3 PLUS a few belonging to that issue PLUS a fix to user module overriding the imported action config entities. Also, we have a WTF: import configuration permission is effin' useless -- you can upload your file but then get a 403 because the sync page requires synchronize configuration. That's a followup to reconsider.

Anonymous’s picture

Status: Needs review » Needs work

absolutely love this patch! chx++

+        file_put_contents('/tmp/log', $e->getMessage());

needs work for that debug.

i like the innovation of sorting the test functions, and reusing the config zip file. that nicely tests 'two completely separate installs'.

i'm not sure if we do this as a follow up or not, but 'two completely separate installs' is just one way to set up envs.

using a db dump and rsync is also a valid use case, so i wonder if we should have a test along those lines as well? i tested this quick hack of ConfigExportImportUITest and it seems to work:

 class ConfigExportImportUITest extends WebTestBase {

  /**
   * @var string
   */
  protected $originalSlogan;

  /**
   * @var string
   */
  protected $newSlogan;

  /**
   * @var string
   */
  protected $tarball;

  public static $modules = array('config');

  public static function getInfo() {
    return array(
      'name' => 'Export/import UI',
      'description' => 'Tests the user interface for importing/exporting configuration.',
      'group' => 'Configuration',
    );
  }

  protected function setUp() {
    parent::setUp();

    $this->drupalLogin($this->root_user);
    $this->originalSlogan = \Drupal::config('system.site')
      ->get('slogan');
    $this->newSlogan = $this->randomString(16);
    \Drupal::config('system.site')
      ->set('slogan', $this->newSlogan)
      ->save();
    $this->drupalPostForm('admin/config/development/configuration/export', array(), 'Export');
    $this->tarball = $this->drupalGetContent();
    \Drupal::config('system.site')
      ->set('slogan', $this->originalSlogan)
      ->save();
  }

  function testImport() {
    $filename = 'temporary://' . $this->randomName();
    file_put_contents($filename, $this->tarball);
    $this->assertNotEqual($this->newSlogan, \Drupal::config('system.site')->get('slogan'));
    $this->drupalPostForm('admin/config/development/configuration/import', array('files[import_tarball]' => $filename), 'Upload');
    $this->drupalPostForm(NULL, array(), 'Import all');
    $this->assertEqual($this->newSlogan, \Drupal::config('system.site')->get('slogan'));
  }
}

this should be another test class, and possibly a follow up commit. will wait for feedback before posting a for-reals patch.

chx’s picture

Status: Needs work » Needs review

I think that's a followup, sorry. This patch is quite badly needed. The real big followup will be setting the testing profile to standard....

webchick’s picture

Status: Needs review » Needs work

I committed #1969800: Add UUIDs to default configuration and #2108419: user module overwrites role actions breaking config sync, so this'll definitely need a re-roll now.

GO CHX GO! :D

chx’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.37 KB

Now the other two is in so YOU SHALL PASS.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

RTBC if it comes back green.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Sorry, not quite. It still needs to pass the docs gate. (Not marking needs work for that though so that we can see what testbot says.)

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.91 KB
1.91 KB

I can't believe we are wasting time on writing doxygen for test methods. Filed #2108785: [policy] Remove the requirement for doxygen for test methods.

chx’s picture

FileSize
4.42 KB
3.17 KB

Interdiff is against #7! Because #7 has passed when this was posted.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome. :)

Committed and pushed to 8.x. Thanks!

webchick’s picture

Title: Write tests for configuration deployment scenarios » Write tests for simple configuration deployment scenario

Updating title to reflect what was actually done here.

webchick’s picture

Issue tags: +Configuration system

.

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