Discovered in #2165549: Reduce number of password hashing iterations in all tests to improve test performance

CommentFileSizeAuthor
#1 drupal8.test-settings.1.patch667 bytessun
drupal8.test-settings.0.patch543 bytessun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new drupal8.test-settings.1.patch667 bytes

KISS? :-)

The last submitted patch, drupal8.test-settings.0.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

The test failure was related to the initial patch that was expected to fail.

Let's move forward with the KISS solution?

dawehner’s picture

dawehner
Primary language German
commented

I always wondered why there is no dedicated method for resetting settings. I guess this is the design of a singleton, much like the german government.
You ensure that there is always one proper instance.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Assigned: Unassigned » sun
sun’s picture

sun
Primary language German
Location Karlsruhe
commented

1: drupal8.test-settings.1.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

This bug also came up in #1808220: Remove run-tests.sh dependency on existing/installed parent site, although rather the "reverse"; i.e., if the test runner has no Settings at all, then various tests are failing, because there is no hash_salt setting. ;) — I've resolved that by instantiating a fake Settings object in run-tests.sh over there.

Would be great to move forward here.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Explanation in #2165549: Reduce number of password hashing iterations in all tests to improve test performance makes sense to me, simple patch => RTBC

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Not downgrading but does this even affect WebTestBase nowadays? I.e. shouldn't the $settings['hash_salt'] be set properly for the child site? In other words, if this only affects DrupalUnitTestBase shouldn't we reset the settings there?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

It affects all tests (and test methods), including WebTestBase.

While it is true that WebTestBase installs a new Drupal site that creates new Settings, the Settings of the test runner would at least leak into the non-interactive installer that is executed by WebTestBase::setUp().

The rule is very simple: All global state constructs MUST be reset in TestBase::prepareEnvironment(). No exceptions.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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