Problem/Motivation

SimpleTest's TestBase class has helper methods to import config and copy config objects. We need the same functionalitity on our PHPUnit Kernel tests and Browser tests.

Proposed resolution

Create a trait to share the code

Remaining tasks

Patch.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#18 config-trait-2759853-18.patch8.45 KBklausi
#12 interdiff.txt1.28 KBklausi
#12 config-trait-2759853-12.patch8.38 KBklausi
#2 config-trait-2759853-2.patch8.24 KBklausi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi created an issue. See original summary.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Active » Needs review
FileSize
config-trait-2759853-2.patch8.24 KB

Patch.

martin107’s picture

martin107 CreditAttribution: martin107 commented

thanks a good idea +1 from me

Patch looks clean

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Reviewed & tested by the community

Let's get this in.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented 
+++ b/core/tests/Drupal/Tests/ConfigTestTrait.php
@@ -0,0 +1,59 @@
+  public function copyConfig(StorageInterface $source_storage, StorageInterface $target_storage) {

Does this has to be public?

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

Probably not, but it was public on Simpletest's TestBase and KernelTestBase, so I would keep it public to avoid an API break for now.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Well, IMHO we can change it to be public later always, but changing from public to protected is more tricky.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Reviewed & tested by the community » Needs review

Yes, and changing it to protected now is tricky because it was public before ;)

Or do you suggest to change the visibility of the method on the old TestBase with

// Change visibility of sayHello
class MyClass1 {
    use HelloWorld { sayHello as protected; }
}

?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Yes, and changing it to protected now is tricky because it was public before ;)

Well for the old testbase we can set them to be public, but for BTB we could leave them as protected.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Needs work

Yes, makes sense!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Cool, thank you!

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs work » Needs review
FileSize
config-trait-2759853-12.patch8.38 KB
interdiff.txt1.28 KB

Done!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Nice! I wasn't even aware that this kind of syntax exists.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: config-trait-2759853-12.patch, failed testing.

The last submitted patch, 12: config-trait-2759853-12.patch, failed testing.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs work » Reviewed & tested by the community

Looks like the random test fail from #2749955: Random fails in UpdatePathTestBase tests, retesting.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Reviewed & tested by the community » Needs work

argl, does not apply anymore.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs work » Needs review
FileSize
config-trait-2759853-18.patch8.45 KB

Simple reroll, no other changes.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Let's move back to RTBC

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

I love this patch!

Committed a1d45d3 and pushed to 8.2.x. Thanks!

  • xjm committed a1d45d3 on 8.2.x 
    Issue #2759853 by klausi, dawehner: Create ConfigTestTrait to share code...

  • xjm committed a1d45d3 on 8.3.x 
    Issue #2759853 by klausi, dawehner: Create ConfigTestTrait to share code...

  • xjm committed a1d45d3 on 8.3.x 
    Issue #2759853 by klausi, dawehner: Create ConfigTestTrait to share code...

Status: Fixed » Closed (fixed)

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