See #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)
Drupal\config\Tests

Skipped tests:

  • ConfigInstallProfileUnmetDependenciesTest (Needs InstallerTestBase)
  • \Drupal\config\Tests\AssertConfigEntityImportTrait could be moved?
  • \Drupal\config\Tests\SchemaCheckTestTrait is deprecated already

Blockers:
#2795085: Add assertNoCacheTag to assertLegacyTrait
#2809471: Convert AJAX part of \Drupal\config\Tests\ConfigEntityTest::testCRUDUI to JavascriptTestBase

A brief history for maintainers, or those wishing to help too by @vaplas:
📌 #8: @andypost created a patch, without going beyond the problem (but with a number of fail places)
📌 #14: @vaplas fixed same fail + added compatibility with Windows (unrelated changes)
📌 #17: @vaplas fixed last fail place + added ConfigEntityTest (because the joy eclipsed him, and he wanted to see all the tests green at once). The patch was unreadable and contained spaghetti from two issues.
📌 #18, #19: @Lendude and @andypost just helped to make it correctly (diff --find-copies).

🎉 Woot! Now we just need to wait while ConfigEntityTest contains js-part. We thought that this would happen quickly, and #21 change status on Postponed, despite the fact that the last patch was spaghetti :)

⏳ But it took some time.

⚔ 23-37: Two new heroes (@jonathan1055 and @Jo Fitzgerald) continue to work with a cold start. Unfortunately, faced with the ConfigEntityTest, they both began to fight with him.

🚩 #28, #35: @Lendude indicates that need a simple port, but in the heat of battle is not understood :)

💡 #27: @jonathan1055 simplifies part of the code, which was approved by dawehner in 39.1.

📌 #38: @vaplas made simple port of ConfigEntityTest.

📌 42: remove unrelated changes for Windows compatibility :)

CommentFileSizeAuthor
#69 interdiff-patches-61-68.txt1.75 KBvaplas
#68 2870439-68.patch26.36 KBandypost
#61 interdiff-59-61.txt611 bytesvaplas
#61 2870439-61.patch26.42 KBvaplas
#59 interdiff-57-59.txt1.36 KBvaplas
#59 2870439-59.patch26.41 KBvaplas
#57 interdiff-53-57.txt1.55 KBvaplas
#57 2870439-57.patch26.57 KBvaplas
#53 interdiff-46-53.txt2.67 KBvaplas
#53 2870439-53.patch25.13 KBvaplas
#51 interdiff-49-51.txt853 bytesvaplas
#51 2870439-51.patch27.71 KBvaplas
#49 interdiff-46-49.txt4.69 KBvaplas
#49 2870439-49.patch27.7 KBvaplas
#48 interdiff-46-48.txt4.69 KBvaplas
#48 2870439-48.patch28.36 KBvaplas
#46 interdiff-42-46.txt2.68 KBvaplas
#46 2870439-46.patch22.93 KBvaplas
#42 interdiff-38-42.txt3 KBvaplas
#42 2870439-42.patch22.96 KBvaplas
#41 2870439-38.interdiff-27-38.jonathan1055.txt11.68 KBjonathan1055
#38 interdiff-27-38.txt1.08 KBvaplas
#38 2870439-38.patch25.06 KBvaplas
#31 2870439-31.patch33.78 KBJo Fitzgerald
#31 interdiff-29-31.txt556 bytesJo Fitzgerald
#29 2870439-29.patch33.7 KBJo Fitzgerald
#29 interdif-27-29.txt12.05 KBJo Fitzgerald
#27 2870439-27.config_tests.patch36.44 KBjonathan1055
#19 2870439-19.patch35.07 KBandypost
#17 interdiff-14-17.txt662 bytesvaplas
#17 2870439-17.patch229.36 KBvaplas
#14 interdiff-8-14.txt8 KBvaplas
#14 2870439-14.patch23.16 KBvaplas
#8 convert_web_tests_to-2870439-8.patch17.78 KBandypost
#8 interdiff.txt5.19 KBandypost
#4 convert_web_tests_to-2870439-4.patch13.91 KBandypost
#2 convert_web_tests_to-2870439-2.patch1.08 KBandypost
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

michielnugter created an issue. See original summary.

andypost’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.08 KB
dawehner’s picture

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

andypost’s picture

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

dawehner’s picture

Issue summary: View changes
Status: Needs review » Needs work

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 ...

andypost’s picture

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

dawehner’s picture

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.

andypost’s picture

Fix few failures, still needs work

dawehner’s picture

Good progress!

Status: Needs review » Needs work

The last submitted patch, 8: convert_web_tests_to-2870439-8.patch, failed testing.

andypost’s picture

naveenvalecha’s picture

Title: Convert web tests to browser tests for config module » [PP-1] Convert web tests to browser tests for config module
Component: phpunit » config.module
Issue summary: View changes
Status: Needs work » Postponed
Issue tags: +phpunit initiative
dawehner’s picture

Title: [PP-1] Convert web tests to browser tests for config module » Convert web tests to browser tests for config module
Status: Postponed » Needs review

This is in, yeah!

vaplas’s picture

Same progress, but still needs work.

#8: agree, #2809471: Convert AJAX part of \Drupal\config\Tests\ConfigEntityTest::testCRUDUI to JavascriptTestBase must really help.

Also few tests not works on Windows:

# Drupal\Tests\config\Functional\ConfigImportUploadTest::testImport()

Warning: unlink(...): Permission denied
Drupal\Core\File\FileSystem->unlink()() (Line: 114)


# Drupal\Tests\config\Functional\ConfigInstallWebTest::testConfigModuleRequirements()

The string "The directory <em class="placeholder">sites/simpletest/23468775/files/config_7IqaXmq53s7yfAxBenWY5d6ay2SmSTzvnQVu29hdb28spHykZtZoZFWtVkTT2ixHBZw6dh4w0w/sync</em> is not writable." was not found anywhere in the HTML response of the current page.

But maybe this can be fixed later.

Status: Needs review » Needs work

The last submitted patch, 14: 2870439-14.patch, failed testing. View results

vaplas’s picture

1) Drupal\Tests\config\Functional\ConfigImportUITest::testImport
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-Drupal
+Config import test o=Q7>&u%

Can anybody reproduce this fail? I have green all tests from ConfigImportUITest. Running tests like:

#Console
php vendor/phpunit/phpunit/phpunit --configuration sites/phpunit.xml.dist core\modules\config\tests\src\Functional\ConfigImportUITest.php
# OK (9 tests, 123 assertions)

#PhpStorm
path\to\php.exe path/to/ide-phpunit.php --configuration path\to\sites\phpunit.xml.dist Drupal\Tests\config\Functional\ConfigImportUITest path\to\core\modules\config\tests\src\Functional\ConfigImportUITest.php
# OK (9 tests, 123 assertions)
vaplas’s picture

Status: Needs work » Needs review
FileSize
229.36 KB
662 bytes

#16.1: Looks like rebuildContainer() solved this problem (see interdiff).
#16.2: Also I included #2809471-13: Convert AJAX part of \Drupal\config\Tests\ConfigEntityTest::testCRUDUI to JavascriptTestBase patch, hoping to get a green color. But maybe we need postponed current issue, by analogy with #2794347-28: Convert web tests to browser tests for field_ui module.

Lendude’s picture

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

andypost’s picture

dawehner’s picture

vaplas’s picture

Title: Convert web tests to browser tests for config module » [PP-1] Convert web tests to browser tests for config module
Status: Needs review » Postponed

Postponed per #8, #17, #20.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jonathan1055’s picture

Title: [PP-1] Convert web tests to browser tests for config module » Convert web tests to browser tests for config module
Issue summary: View changes
Status: Postponed » Active
jonathan1055’s picture

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).

naveenvalecha’s picture

Status: Active » Closed (duplicate)

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

jonathan1055’s picture

Status: Closed (duplicate) » Needs work

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.

jonathan1055’s picture

Status: Needs work » Needs review
FileSize
36.44 KB

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.

Lendude’s picture

Status: Needs review » Needs work

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.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
12.05 KB
33.7 KB

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

Status: Needs review » Needs work

The last submitted patch, 29: 2870439-29.patch, failed testing. View results

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
556 bytes
33.78 KB

Add missing use statement.

Status: Needs review » Needs work

The last submitted patch, 31: 2870439-31.patch, failed testing. View results

jonathan1055’s picture

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

Jo Fitzgerald’s picture

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

Lendude’s picture

+++ 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?

jonathan1055’s picture

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

Jo Fitzgerald’s picture

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.

vaplas’s picture

Status: Needs work » Needs review
FileSize
25.06 KB
1.08 KB

Thanks @jonathan1055 and @Jo Fitzgerald!
I also worked on refactoring ConfigEntityTest in #2809471-19: Convert AJAX part of \Drupal\config\Tests\ConfigEntityTest::testCRUDUI to JavascriptTestBase :) But then after dawehner's advice, we found an easier way of porting js part.

#18, #19: @Lendude, @andypost thanks for hint and hep with make it.

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

dawehner’s picture

  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?

jonathan1055’s picture

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.

jonathan1055’s picture

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]

vaplas’s picture

#39.1: yep, @jonathan1055 added this in the #27.
#39.2: fair! I added this for compatibility with Windows, and remove these unrelated changes now. Because the problem with incompatibilities between OS is quite common, and we can do it after porting.

#40-41: you caught me!) All your questions are fair, because I really made a mess with description in my last posts. Sorry! Now interdiff is true interdiff :)

A brief history for mantainers, or those wishing to help too:
📌 #8: @andypost created a patch, without going beyond the problem (but with a number of fail places)
📌 #14: @vaplas fixed same fail + added compatibility with Windows (unrelated changes)
📌 #17: @vaplas fixed last fail place + added ConfigEntityTest (because the joy eclipsed him, and he wanted to see all the tests green at once). The patch was unreadable and contained spaghetti from two issues.
📌 #18, #19: @Lendude and @andypost just helped to make it correctly (diff --find-copies).

🎉 Woot! Now we just need to wait while ConfigEntityTest contains js-part. We thought that this would happen quickly, and #21 change status on Postponed, despite the fact that the last patch was spaghetti :)

⏳ But it took some time.

⚔ 23-37: Two new heroes (@jonathan1055 and @Jo Fitzgerald) continue to work with a cold start. Unfortunately, faced with the ConfigEntityTest, they both began to fight with him.

🚩 #28, #35: @Lendude indicates that need a simple port, but in the heat of battle is not understood :)

💡 #27: @jonathan1055 simplifies part of the code, which was approved by dawehner in 39.1.

📌 #38: @vaplas made simple port of ConfigEntityTest.

📌 42: remove unrelated changes for Windows compatibility :)

To be continued?)

jonathan1055’s picture

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’s picture

@jonathan1055, thank you for double-check and such kind feedback!

I also noticed that @dawehner indicated in 39.2 one more item:

+++ b/core/modules/config/tests/src/Functional/ConfigInstallWebTest.php
@@ -156,7 +157,7 @@ public function testPreExistingConfigInstall() {
-    $this->drupalGet($this->getAbsoluteUrl($url));
+    $this->drupalGet($this->getAbsoluteUrl($url->getText()));

We need this, because the following code gives different results between simpletest and phpunit:

$url = $this->xpath("//a[contains(@href,'config_clash_test_theme') and contains(@href,'/install?')]/@href")[0];
// simpletest return \SimpleXMLElement[], where [0] is string value (via __toString()).
// phpunit return \Behat\Mink\Element\NodeElement[], where [0] is object value.

So, we can replace this code on a more semantically understandable, like

- $url = $this->xpath("//a[contains(@href,'config_clash_test_theme') and contains(@href,'/install?')]/@href")[0];
- $this->drupalGet($this->getAbsoluteUrl($url));
+ $href = $this->xpath("//a[contains(@href,'config_clash_test_theme') and contains(@href,'/install?')]")[0]->getAttribute('href');
+ $this->drupalGet($this->getAbsoluteUrl($href));

But perhaps this is beyond minimal porting, so @andypost made a simplest replacement.

Additional review:

  1. +++ b/core/modules/config/tests/src/Functional/ConfigEntityFormOverrideTest.php
    @@ -46,7 +46,7 @@ public function testFormsWithOverrides() {
    -    $this->assertIdentical((string) $elements[0]['value'], $original_label);
    +    $this->assertIdentical((string) $elements[0]->getValue(), $original_label);
    
    @@ -61,7 +61,7 @@ public function testFormsWithOverrides() {
    -    $this->assertIdentical((string) $elements[0]['value'], $edited_label);
    +    $this->assertIdentical((string) $elements[0]->getValue(), $edited_label);
    
    +++ b/core/modules/config/tests/src/Functional/ConfigFormOverrideTest.php
    @@ -32,7 +32,7 @@ public function testFormsWithOverrides() {
    -    $this->assertIdentical((string) $elements[0]['value'], 'Drupal');
    +    $this->assertIdentical((string) $elements[0]->getValue(), 'Drupal');
    
    @@ -41,7 +41,7 @@ public function testFormsWithOverrides() {
    -    $this->assertIdentical((string) $elements[0]['value'], $edit['site_name']);
    +    $this->assertIdentical((string) $elements[0]->getValue(), $edit['site_name']);
    
    

    We can do without (string) conversions:

    - (string) $elements[0]['value']
    + $elements[0]->getValue()
    
  2. +++ b/core/modules/config/tests/src/Functional/ConfigImportUITest.php
    @@ -130,6 +130,7 @@ public function testImport() {
    $this->drupalPostForm(NULL, [], t('Import all'));
    ...
    +    $this->rebuildContainer();
    

    We need rebuildContainer() to update the container variable, after import operations, otherwise the fail in test. (see #15 - #17).

  3. +++ b/core/modules/config/tests/src/Functional/ConfigInstallWebTest.php
    @@ -51,6 +51,7 @@ public function testIntegrationModuleReinstallation() {
    +    $this->resetAll();
    

    We need resetAll() after install the integration module, otherwise the fail in test.

  4. +++ b/core/modules/config/tests/src/Functional/ConfigSingleImportExportTest.php
    @@ -225,16 +225,16 @@ public function testExport() {
    -    $element = $this->xpath('//select[@name="config_name"]');
    -    $options = $this->getAllOptions($element[0]);
    +    $element = $this->xpath('//select[@name="config_name"]')[0];
    +    $options = $element->findAll('css', 'option');
    

    Hm.. after #2907485: Add getAllOptions() to AssertLegacyTrait we can do without it, but once already done, why revert it to deprecated state?)

Looks like all other changes is a direct code transfer (except #39.1, of course).

Lendude’s picture

Issue summary: View changes

@vaplas++++++

added your summary to the IS

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

vaplas’s picture

Lendude’s picture

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?

vaplas’s picture

FileSize
28.36 KB
4.69 KB

What new place will be best? SchemaCheckTestTrait use core/tests/Drupal/Tests location. But, as you pointed out, the AssertConfigEntityImportTrait is more specific. So I put it in the core/modules/config/tests/src/Traits (simple c/p + update EntityReferenceIntegrationTest).

vaplas’s picture

FileSize
27.7 KB
4.69 KB

Opps, incorrect changes. Ingnore #48 patch, please.

andypost’s picture

+++ 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

vaplas’s picture

Done.

Lendude’s picture

+++ 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.

vaplas’s picture

#52: it is logical, thanks @Lendude! Back to #46 and try again). So, now #50 applied only to phpunit version.

Status: Needs review » Needs work

The last submitted patch, 53: 2870439-53.patch, failed testing. View results

jonathan1055’s picture

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 ;-)

jonathan1055’s picture

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

vaplas’s picture

Status: Needs work » Needs review
FileSize
26.57 KB
1.55 KB

@jonathan1055, thank you for clarifying and CR!
Did I understand correctly that we need to update the EntityReferenceIntegrationTest (done), or should we not add a trigger_error to simpletest AssertConfigEntityImportTrait?

I also overlooked #52.1. Done.

jonathan1055’s picture

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

vaplas’s picture

Indeed! + change 8.4.x on 8.4.1, I hope this is really quite plausible version ;)

jonathan1055’s picture

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 ;-)

vaplas’s picture

FileSize
26.42 KB
611 bytes

@jonathan1055, thanks again! Catches from others can not stop re-roll after good catch from you ;)

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

Looks great now!

alexpott’s picture

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

alexpott credited GoZ.

alexpott’s picture

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

alexpott’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

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.

  • alexpott committed 093b148 on 8.5.x
    Issue #2870439 by vaplas, andypost, Jo Fitzgerald, jonathan1055,...
andypost’s picture

Status: Patch (to be ported) » Needs review
FileSize
26.36 KB

Quick re-roll for 8.4.x

vaplas’s picture

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

🎉🎉🎉

#68: I can confirm, that it is re-roll, and it is very quick!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed 75273e5 on 8.4.x
    Issue #2870439 by vaplas, andypost, Jo Fitzgerald, jonathan1055, Lendude...
jonathan1055’s picture

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

jonathan1055’s picture

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

andypost’s picture

CR published

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

jonathan1055’s picture

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

Status: Fixed » Closed (fixed)

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