The @backupStaticAttributes and @backupGlobals attributes should not be used in pure unit tests.

Unit tests should explicitly manage the state of global and static variables under test.

The two features are only meant for functional/integration testing (as well as testing legacy code bases), but not for pure unit tests. Especially the behavior of @backupStaticAttributes is hard to predict and not documented yet - which is documented now.

$ grep -r --include='*Test.php' --exclude-dir=vendor '@backup' core
core/tests/Drupal/Tests/Core/Form/FormCacheTest.php: * @backupStaticAttributes enabled

Task

  1. Figure out what statics need to be reset in affected test(s).

  2. Reset them both in setUp() and tearDown().

The fact that the global state dependencies of affected tests are unknown further underlines why the @backup* features should not be used in (pure) unit tests.

CommentFileSizeAuthor
#1 phpunit-2329765-1.patch1.49 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
phpunit-2329765-1.patch1.49 KB

The usage of it in FormCacheTest is not "unknown". It's for SafeMarkup, which has (and needs) no way to reset the seen IDs.

sun’s picture

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

Thanks! I did hope that you'd remember why @backupStaticAttributes is used in that recently added test. :)

I don't know why SafeMarkup needs a static like that and why it can't use a stack/collection object, but it's good that the issue is exposed now.

PHPUnit's documentation has been clarified; the improvements will be deployed in the next days.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

FWIW, run-tests.sh and the testbot do not reveal such leaking global state currently, because unit tests are effectively executed as if --process-isolation was used for PHPUnit (which is terribly slow and thus strongly not recommended).

Process isolation means that every test (class, in case of run-tests.sh) is executed in a separate process. When reloaded from scratch in a separate process, static class properties do have their originally declared default values.

However, process isolation is only meant to be used for legacy code that cannot be refactored/tested through other means, or for testing internal PHP constructs that cannot be isolated at all (such as testing a class loader).

Regular unit tests should not use or depend on process isolation. The current execution of unit tests in run-tests.sh is a straight bug, caused by its legacy architecture. That difference results in test errors when running phpunit from the command line (which intentionally does not use process isolation by default).

The 12 errors that are currently reported by phpunit are a testament to that:

1) Drupal\Tests\Core\Form\FormCacheTest::testGetCacheValidToken
...
12) Drupal\Tests\Core\Form\FormCacheTest::testSetCacheWithSafeStrings
serialize(): __sleep should return an array only containing the names of instance-variables to serialize

The errors are caused by @backupStaticAttributes, because it tries to serialize every value of every static property in each and every class that happens to be declared.

  1. This patch fixes that.
  2. The (single) reported error means that there is a broken __sleep() function somewhere in our code, but that's a separate issue, unrelated to this issue here.
  3. I don't plan to fix run-tests.sh, my goal is to get rid of it entirely for 8.0.0.
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue summary: View changes
neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

its legacy architecture.

Yeah, legacy and incremental design added to tight coupling to the web runner which is effectively our first class runner. I never considered removing the script because I find being able to somewhat replicate the testbots locally is useful but I have toyed with it several times. Not breaking the webrunner in the process has always tripped me up so I'm interested in your plans.

Anyways, big +1 and RTBC again if that's possible. Soon if possible because running tests in PhpStorm is annoying with these failures.

webchick’s picture

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

No idea what this does, but I guess it's safe if sun and tim agree. :)

Committed and pushed to 8.x. Thanks!

  • webchick committed 7c79e1c on 8.0.x 
    Issue #2329765 by tim.plunkett | sun: Remove all @backup* annotations...

Status: Fixed » Closed (fixed)

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