CommentFileSizeAuthor
#18 drupal8.filter-admintest.18.patch3.08 KBsun
#5 interdiff.txt501 bytessun
#5 drupal8.filter-admintest.5.patch3.07 KBsun
#2 filter-2254183-2.patch3.12 KBtim.plunkett
testperf.filter-FilterAdminTest.patch1.8 KBsun

Comments

Status: Needs review » Needs work

The last submitted patch, testperf.filter-FilterAdminTest.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new filter-2254183-2.patch3.12 KB

We need to set those formats up now.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterAdminTest.php
@@ -29,13 +30,63 @@ public static function getInfo() {
+    drupal_static_reset('checkPermissions');

The "API" here is rather

$this->checkPermissions(array(), TRUE);

...but that can be fixed later.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Created #2256317: Remove static caching from WebTestBase::checkPermissions()

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new drupal8.filter-admintest.5.patch3.07 KB
new interdiff.txt501 bytes

That has landed, so we can simply remove that drupal_static_reset() now. :-)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: drupal8.filter-admintest.5.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

5: drupal8.filter-admintest.5.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Reviewed & tested by the community

Created #2258161: Random test failure in Drupal\language\Tests\LanguageFallbackTest

dries’s picture

dries commented

How do we know the performance improvement is worth the extra complexity/brittleness?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

I would say there is *less* brittleness, because we're no longer relying on the needs of standard profile to test our APIs.
We're now explicitly setting up our expectations.

And I think adjusting this test if we ever want to adjust the filters we ship with would be more complex.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

This issue is part of an effort to clean up the last remaining tests that still depend on the Standard profile.

Yes, this is not only improving performance, it is also decoupling a functional module test from Standard profile/product expectations.

Only tests of the Standard profile itself should use the Standard profile in tests. That is, because only Standard profile knows its own expectations.

All other tests should not make any assumptions about possibly existing default configuration of an(y) installation profile. That's why we introduced the Testing profile, which ships with no modules and no configuration, so module tests can start off a clean slate.

I do agree that the necessary code to set up test fixtures is very verbose and we should simplify and improve that. To do so, I recently posted a revised proposal in #913086-112: Allow modules to provide default configuration for running tests

However, all other tests in HEAD are setting up fixtures as in this patch, so that shouldn't hold up this patch.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Back to Dries. Though for my part, I would vastly prefer to see this handled via #2096899: Add $EntityType::create() to simplify creating new entities (which both test authors and distro authors could use) than repeating this verbose scaffolding everywhere.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Assigned: Unassigned » dries
sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Clarification here: #913086-116: Allow modules to provide default configuration for running tests

Not really on-topic/in-scope for this issue here. This patch just does what all other tests in HEAD are doing already.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

From my comment in the issue that webchick mentioned above (#2096899-24: Add $EntityType::create() to simplify creating new entities):

As discussed in the other issue, this will *not* make that issue and similar ones easier, all it will do is change entity_create('filter_format', ...) to FilterFormat::create(...), everything else will stay the same, the complexity that you see there are the filter format data structures, not the API. I fully agree with @sun and @timplunkett over there that it makes the test less brittle and more explicit about what it is testing instead of relying on some default configuration somewhere.

The change is in line with what we've been doing in many other tests and while it is ugly, that is not the fault of entity API's but the problem of missing standardized test fixtures as @sun pointed out. And we apparently even have an issue to address that ;)

catch’s picture

catch
he/him
Primary language English
commented

Yes agreed with berdir/sun, this is verbosity rather than complexity and it makes the test less brittle rather than more.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: drupal8.filter-admintest.5.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new drupal8.filter-admintest.18.patch3.08 KB

Re-rolled against HEAD.

dries’s picture

dries commented
Assigned: dries » Unassigned
Status: Reviewed & tested by the community » Fixed

Looked at it more and it actually is better. Committed to 8.x. Thanks.

  • Commit f03e93c on 8.x by Dries: 
    Issue #2254183 by sun, tim.plunkett: Fix test performance of Drupal\...

Status: Fixed » Closed (fixed)

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