Convert web tests to browser tests for config module

What do we do with all the other files in this directory?

Sorry, now the patch that moves all tests except ConfigInstallProfileUnmetDependenciesTest and 2 traits

I agree, this test doesn't belong here yet. One of the traits is already deprecated, the other one isn't. We should move it && refer from the old one, just like \Drupal\config\Tests\SchemaCheckTestTrait is done. Note: We probably want to move it to core/modules/config/tests ...

Good idea, but SchemaCheckTestTrait used in \Drupal\views\Tests\Handler\FilterDateTest

PS: also it's used in \Drupal\page_manager\Tests\PageConfigSchemaTest so needs other issue

Good idea, but SchemaCheckTestTrait used in \Drupal\views\Tests\Handler\FilterDateTest

No worries, this is why we have deprecations, don't we?

Please keep in mind: try to make as few changes as possible so the patches are in a reviewable form.

Fix few failures, still needs work

Good progress!

This issue is blocked by #2795085: Add assertNoCacheTag to assertLegacyTrait

This is in, yeah!

@vaplas think you missed --find-copies when rolling that patch :) Could you reroll?

here's reroll

Should we wait until #2809471: Convert AJAX part of \Drupal\config\Tests\ConfigEntityTest::testCRUDUI to JavascriptTestBase is in?

#2809471: Convert AJAX part of \Drupal\config\Tests\ConfigEntityTest::testCRUDUI to JavascriptTestBase is in, so un-postponing.

It looks like we have duplicate issues for converting the config tests:

#2864026: Convert web tests to browser tests for config module
Created on 25 March 2017.
It is a child of #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) but is not shown on that issue summary.

#2870439: Convert web tests to browser tests for config module (this issue)
Created 17 April 2017.
Also a child of the main issue and is shown in that issue summary. It has a bigger patch with more work done (15 files changed compared to 10).

Closing this as duplicate in favor of #2864026: Convert web tests to browser tests for config module Thanks for the work here @andypost and @valpas

Thanks naveenvalecha. However, I think it should be the other way round. I have downloaded, tested and compared the two patches, and the work in this issue has progressed further, even though it started later. The work does differ slightly, so we can discuss which bits to use from each, but this is the issue which should remain open, and #2864026: Convert web tests to browser tests for config module should be closed as a duplicate.

The patch in #19 does not apply anymore, so I will upload a re-roll shortly, then we can go from there.

Here is the patch from #19 re-rolled. It was clear how to correct it for most of the source commits which had happened since July. However, I have some difficulty with testCRUD in /Functional/ConfigEntityTest.php. Also the file src/FunctionalJavascript/ConfigEntityTest.php now already exists, so I have temporarily called the new one created in #19 /ConfigEntityTestNew.php. This will have to change, and looking at the comments, it might have been copy'n'pasted, because the @group is views_ui. Is that correct?

I will provide an interdiff shortly, however, I want to get this patch up and tested on d.o. Two of these tests fail on my localhost but they may run OK here.

What remains of ConfigEntityTest in HEAD can now just be converted to a normal Functional test, the part that needed Javascript was removed in #2809471: Convert AJAX part of \Drupal\config\Tests\ConfigEntityTest::testCRUDUI to JavascriptTestBase.

Combined FunctionalJavascript/ConfigEntityTestNew.php with Functional/ConfigEntityTest.php.

Add missing use statement.

Hi Jo,
Are you sure that this test code is still actually needed? I moved it there as a temporary measure to get the re-roll done, and make sure nothing got accidentally dropped, but see Lendude's comment:

What remains of ConfigEntityTest in HEAD can now just be converted to a normal Functional test

I took the remainder and moved it into a normal functional test, which is already in patch #27. We need to examine the code in detail, but at first glance it looks like all the javascript testing is already in #2809471: Convert AJAX part of \Drupal\config\Tests\ConfigEntityTest::testCRUDUI to JavascriptTestBase which created the new src/FunctionalJavascript/ConfigEntityTest.php. So maybe we do not need to add anything more to the javascript test?

Jonathan

Oh, I thought that was the remaining code to which @Lendude was referring. Perhaps not.

+++ b/core/modules/config/tests/src/FunctionalJavascript/ConfigEntityTestNew.php
@@ -0,0 +1,160 @@
+class ConfigEntityTestNew extends JavascriptTestBase {

What I meant was that in #27 I would say that we just need to change this to extends BrowserTestBase and not JavascriptTestBase. So basically treat ConfigEntityTest like any other test we are converting, since the part that needed javascript has already been removed from the test.

Does that make sense?

Yes, and I think I have already done that:

diff --git a/core/modules/config/src/Tests/ConfigEntityTest.php b/core/modules/config/tests/src/Functional/ConfigEntityTest.php
similarity index 73%
rename from core/modules/config/src/Tests/ConfigEntityTest.php
rename to core/modules/config/tests/src/Functional/ConfigEntityTest.php
index a0c857b..bd07791 100644
--- a/core/modules/config/src/Tests/ConfigEntityTest.php
+++ b/core/modules/config/tests/src/Functional/ConfigEntityTest.php
@@ -1,6 +1,6 @@
 <?php
 
-namespace Drupal\config\Tests;
+namespace Drupal\Tests\config\Functional;
 
 use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Core\Entity\EntityMalformedException;
@@ -8,14 +8,14 @@
 use Drupal\Core\Config\Entity\ConfigEntityStorage;
 use Drupal\Core\Config\Entity\Exception\ConfigEntityIdLengthException;
 use Drupal\Core\Url;
-use Drupal\simpletest\WebTestBase;
+use Drupal\Tests\BrowserTestBase;
 
 /**
  * Tests configuration entities.
  *
  * @group config
  */
-class ConfigEntityTest extends WebTestBase {
+class ConfigEntityTest extends BrowserTestBase {
 

Beneath this, you can see where I removed lots of code from testCRUDUI(). So possibly the only thing that needs to be done with patch #27 is just delete the creation of file FunctionalJavascript/ConfigEntityTestNew.php

Jonathan

This was my thought process:

1. FunctionalJavascript/ConfigEntityTestNew.php should extend BrowserTestBase
2. So it should be Functional/ConfigEntityTestNew.php
3. It no longer needs the "New" suffix
4. So it should be Functional/ConfigEntityTest.php
5. But Functional/ConfigEntityTest.php already exists
6. So the 2 should be combined
7. But they both contain testCRUDUI()
8. So both versions of the method should be combined

That's how I got to the conclusion I did. Sorry if I got it wrong.

EDIT: Cross-post.

1. +++ b/core/modules/config/tests/src/Functional/ConfigExportUITest.php
   @@ -44,7 +44,8 @@ public function testExport() {
   -    // Submit the export form and verify response.
   +    // Submit the export form and verify response. This will create a file in
   +    // temporary directory with the default name config.tar.gz.
        $this->drupalPostForm('admin/config/development/configuration/full/export', [], t('Export'));
        $this->assertResponse(200, 'User can access the download callback.');
    
   @@ -55,14 +56,8 @@ public function testExport() {
   
   @@ -55,14 +56,8 @@ public function testExport() {
        $header_match = (boolean) preg_match('/attachment; filename="config-' . preg_quote($hostname) . '-\d{4}-\d{2}-\d{2}-\d{2}-\d{2}\.tar\.gz"/', $header_content_disposition);
        $this->assertTrue($header_match, "Header with filename matches the expected format.");
    
   -    // Get the archived binary file provided to user for download.
   -    $archive_data = $this->getRawContent();
   -
   -    // Temporarily save the archive file.
   -    $uri = file_unmanaged_save_data($archive_data, 'temporary://config.tar.gz');
   -
        // Extract the archive and verify it's not empty.
   -    $file_path = file_directory_temp() . '/' . file_uri_target($uri);
   +    $file_path = file_directory_temp() . '/' . 'config.tar.gz';
        $archiver = new Tar($file_path);
   

   👍 One less workaround we have laying around!

2. +++ b/core/modules/config/tests/src/Functional/ConfigInstallWebTest.php
   @@ -148,7 +149,8 @@ public function testPreExistingConfigInstall() {
   -    $this->assertRaw('Unable to install Configuration install fail test, <em class="placeholder">config_test.dynamic.dotted.default, language/fr/config_test.dynamic.dotted.default</em> already exist in active configuration.');
   +    $placeholder = str_replace('/', DIRECTORY_SEPARATOR, 'language/fr/config_test.dynamic.dotted.default');
   +    $this->assertRaw('Unable to install Configuration install fail test, <em class="placeholder">config_test.dynamic.dotted.default, ' . $placeholder . '</em> already exist in active configuration.');
   
   @@ -156,8 +158,9 @@ public function testPreExistingConfigInstall() {
   -    $this->drupalGet($this->getAbsoluteUrl($url));
   -    $this->assertRaw('Unable to install config_clash_test_theme, <em class="placeholder">config_test.dynamic.dotted.default, language/fr/config_test.dynamic.dotted.default</em> already exist in active configuration.');
   +    $this->drupalGet($this->getAbsoluteUrl($url->getText()));
   +    $placeholder = str_replace('/', DIRECTORY_SEPARATOR, 'language/fr/config_test.dynamic.dotted.default');
   +    $this->assertRaw('Unable to install config_clash_test_theme, <em class="placeholder">config_test.dynamic.dotted.default, ' . $placeholder . '</em> already exist in active configuration.');
   
   @@ -167,7 +170,8 @@ public function testPreExistingConfigInstall() {
   -      $this->assertEqual($e->getMessage(), 'Configuration objects (config_test.dynamic.dotted.default, language/fr/config_test.dynamic.dotted.default) provided by config_clash_test_theme already exist in active configuration');
   +      $placeholder = str_replace('/', DIRECTORY_SEPARATOR, 'language/fr/config_test.dynamic.dotted.default');
   +      $this->assertEqual($e->getMessage(), 'Configuration objects (config_test.dynamic.dotted.default, ' . $placeholder . ') provided by config_clash_test_theme already exist in active configuration');
        }
   

   ❓These look like unrelated changes ... do you mind clarifying?

Thanks valpas. Your patch is OK in principle, but it does not actually match your interdiff

#35: done! The patch is based on #27 + port ConfigEntityTest as is.

I had already started the WTB -> BTB conversion for ConfigEntityTest mentioned by Lendude in 35 - see my comment in 36. So what your inderdiff should show is the lines removed/added in testCRUDUI().

You have correctly removed the unwanted new file FunctionalJavascript/ConfigEntityTestNew.php. Thanks.

Here is the actual interdiff between patches #27 and #38.

Now that we have Valpas here, who worked on #2809471: Convert AJAX part of \Drupal\config\Tests\ConfigEntityTest::testCRUDUI to JavascriptTestBase I am sure we can get this sorted. What you are saying (and the patch in #38 matches this) is that we do not need any of the material changes within the tests in ConfigEntityTest we can take it exactly as-is and just do the straight WTB->BTB conversion. In that case patch 38 is good (apart from the review points noted by dawehner in #39)

[edit: your comment and interdiff confused me, but your work is good :-) Sorry for my lack of understanding, I have now realised what you meant, and the patch in 38 is right]

Valpas you are a star! What an excellent summing up of the story.

I've compared your patch 42 with 38 and the three sets of changes in ConfigInstallWebTest.php do indeed revert those lines back to unchanged, as dawehner asked for.

@vaplas++++++

added your summary to the IS

I think the only thing left to do is #44.1? Rest looks great to me.

Looks great now.

So to me the only question remaining is what to do about AssertConfigEntityImportTrait.

The IS says 'move?', I'd say 'copy to new location and deprecate the old one'. It is only used once in core, so it won't inflate this patch too much. Other ideas?

+++ b/core/modules/config/tests/src/Traits/AssertConfigEntityImportTrait.php
@@ -0,0 +1,37 @@
+    $imported_entity = \Drupal::entityManager()->loadEntityByUuid($entity_type_id, $entity_uuid);

I bet new code should not use deprecated EM

+++ b/core/modules/config/tests/src/Traits/AssertConfigEntityImportTrait.php
@@ -0,0 +1,37 @@
+ * Can be used by test classes that extend \Drupal\simpletest\WebTestBase or
+ * \Drupal\KernelTests\KernelTestBase.

Since it's now used in a BrowserTestBase test, this comment is no longer accurate

+++ b/core/modules/config/src/Tests/AssertConfigEntityImportTrait.php
@@ -2,36 +2,16 @@
+  use \Drupal\Tests\config\Traits\AssertConfigEntityImportTrait;

And are we sure we want to make the old trait just use the new trait? That way we can't make any changes to the trait that are PHPUnit specific without breaking the trait meant for WebTestBase. I would opt to leave the old trait as is, just deprecated. Yeah it gives some code duplication now, but it means we can do further changes to the new trait without fear of breaking Simpletest tests.
Not sure what is preferable here.

Hi valpas,
In patch #53 was the failure due to not making the change in Functional/EntityReference/EntityReferenceIntegrationTest.php as was done in patch #51 ?

-use Drupal\config\Tests\AssertConfigEntityImportTrait;
...
+use Drupal\Tests\config\Traits\AssertConfigEntityImportTrait;

Hence the test produced the deprecation message and failed exactly as we would want, when using deprecated code ;-)

We also need a Change Record for the deprecation, which I have just created. I set the "introduced in version" to 8.4.1 given than 8.4.0 is now released.

https://www.drupal.org/node/2916197

Did I understand correctly that we need to update EntityReferenceIntegrationTest

Yes, you did the correct option. The test needs to use the new trait, and the deprecation trigger_error should stay, so that any other newly created tests produce an error if they start using it.

You can now also add the CR url into the @trigger_error message and the @deprecated tag, as per https://www.drupal.org/core/deprecation

Looking good. One minor point - you missed the id in the docblock url

+ * @see https://www.drupal.org/node

Don't re-roll the patch yet, as others may spot something else ;-)

Looks great now!

Adding credit for @dawehner and @Lendude for patch reviews.

Adding credit to @GoZ for working on the duplicate issue #2864026: Convert web tests to browser tests for config module

Committed 093b148 and pushed to 8.5.x. Thanks!

We need a quick patch for 8.4.x because there are conflicts in ConfigEntityListTest due to #2767857: Add destination to edit, delete, enable, disable links in entity list builders.

Quick re-roll for 8.4.x

Committed 75273e5 and pushed to 8.4.x. Thanks!

Thanks for the super quick-reroll @andypost. And @vaplas that diff is helpful. It's great to be able to keep the branches in-sync for tests.

Thanks alexpott and everyone. Good team effort.
The change record I created is still in draft. Should that be published now? https://www.drupal.org/node/2916197

Interesting that the "View all draft change records" link is actually wrong. That url https://www.drupal.org/list-changes/drafts tries to display all change records for the "draft" project. It should actually be https://www.drupal.org/list-changes/drupal/drafts

CR published

@jonathan1055 follow the issue #2151041-92: Add a "draft" status for change records

Thanks andypost for raising it on that issue, which I have now followed.