backupStaticAttributes doesn't work properly when class autoloaders are in use (was #1)

[RichardBradley]

(Moved from issue #1, due to that issue repeatedly being incorrectly auto-closed by GitHub.)

Summary

• PHPUnit has a feature "backupStaticAttributes" which is documented to "run your tests in a way where changes to [static] variables do not affect other tests"
• The implementation of this feature works by enumerating all loaded classes and copying the "before" values of their static fields into an array, then restoring those values after each test has run
• However, this doesn't work when class autoloaders are in use, because "get_declared_classes()" is called by the "backupStaticAttributes" feature before the test is run. If the test causes a new class to be auto-loaded during the test run, then its static fields will not be included in the "before" values, and any value which is added to a static field by that test will leak into all other tests for the rest of the suite run.

Example use case

You may dismiss this by saying that the static field didn't exist before the test was run, so "backupStaticAttributes" couldn't catch it, but that does not account for how this feature appears to be intended to operate from the point of view of a PHPUnit user.

If I have a static class which looks like, for example:

  class MyGlobalStateHolder {
    static $scriptStartTime = time()
  }

... and if I want to modify this static field for the duration of a single test:

  class MyTimeDependentTest {
    protected function setUp() {
      // for the duration of this test only, pretend that it is 1984
      MyGlobalStateHolder::$scriptStartTime = 441763200;
    }
  }

Then, as things are documented I should expect the "backupStaticAttributes" feature to cause this field change to be isolated to this single test. This bug means that whichever test first loads the class in question will "win" and its static changes will leak to all other tests in the same run.

Possible fixes and workarounds

It is not clear to me that this is easily fixable, but it is a real bug.

I can't see a "class loaded" event to which PHPUnit could subscribe. Perhaps @eriksencosta 's suggestion of wrapping the autoloader would work. However, any autoloaders added after the wrapping occurred would be missed, which seems quite likely to happen (e.g. Zend controller tests bootstrap Zend in the test "setup" method, at which point a lot of autoloaders are added).

Perhaps the fix should be documentation of the limitations of the "backupStaticAttributes" feature. or perhaps the "backupStaticAttributes" feature needs to be removed as not workable.

Perhaps the restoreStaticAttributes() method could check whether any new classes have been autoloaded in the course of the test and, if so, issue a warning that "backupStaticAttributes" doesn't work in the presence of autoloaders?

That wouldn't fix the issue, but at least it would give a warning when it occurred, which might save people lots of time tracking down mysterious bugs that depend on the order in which tests are run.

Isolated test case

See https://github.com/thehereward/phpunit-backup-static-attributes-bug

[sun]

get_declared_classes() is called by the "backupStaticAttributes" feature before the test is run. If the test causes a new class to be auto-loaded during the test run, then its static fields will not be included in the "before" values, and any value which is added to a static field by that test will leak into all other tests for the rest of the suite run.

Consequently, all statics of newly loaded classes were "empty" (as declared by default) before the test was run, and thus, the statics of newly loaded classes should be reset/reverted to their declared default values.

Comparing the list of get_declared_classes() after each test with the list from before the test + resetting new statics accordingly sounds sensible to me.

This process needs to skip class properties defined in $backupStaticAttributesBlacklist.

[RichardBradley]

Yes, that would work, if PHPUnit can access the "declared default values". Did you have an implementation in mind for that? (It seems like a rather tricky problem to me, but there's probably something I haven't thought of.)

[sun]

Hm… I thought of ReflectionClass::getDefaultProperties(), but apparently there's a warning:

The default value of a static class property can not be tracked when using this method on user defined classes.

Not sure whether that means what it seems to mean; needs manual testing, I guess.

[RichardBradley]

needs manual testing, I guess.

It works on HHVM, but doesn't work on PHP 5

http://3v4l.org/ZduM4

[sun]

Looks like there's a bug in HHVM 😉

Just tried various options myself, but yes, PHP does not seem to record/track the original default value of static class properties (most likely, all static variables in general).

In turn, it looks like there are only two options:

1. Performance drain, but for completeness:

   Execute a super-minimal child process after executing a test, which…

   1. loads newly loaded classes (direct require on known file paths)
   2. retrieves the default values of static properties
   3. returns the result.

   Only executed if a newly loaded class has a static property.

2. Highly complex:

   1. Wrap all registered classloaders into a custom
   2. record the declared default value of static properties before any other code runs.

[RichardBradley]

(1) Execute a super-minimal child process after executing a test

This seems too heavyweight to me to be workable.

Don't forget that "backupStaticAttributes" is positioned as an alternative to "--process-isolation", which executes each test in its own process, guaranteeing isolation of static variables (but at a huge performance cost, especially on windows).

(2) Wrap all registered classloaders into a custom

I don't think this is possible, see my comment re Zend autoloaders on the old issue thread.

I can't see a realistic fix for this issue. I think PHPUnit should issue a warning whenever "backupStaticAttributes" is set and it detects that a class was loaded during the course of a test (and that class has static fields):

WARNING: The class "ClassA" was loaded during the course of test "YourTest",
and contains static attributes. The "backupStaticAttributes" feature cannot backup
static attributes of classes which are loaded during the course of a test run. To
use this feature, all classes must be loaded in advance of the test. You can work
around this issue by pre-loading the class in your test boostrap file, e.g.

class_exists('ClassA');

[sun]

Created sebastianbergmann/phpunit-documentation#226 to at least address this in the manual for the time being.

Now, potentially silly consideration. Theoretically…

1. Unit tests are supposed to cover your entire code base.
2. With a 100% coverage, running unit tests will load your entire code base anyway.
3. Why don't we load each and everything upfront + backup all statics once ahead of time, before executing any tests and user-space code?

Obvious constraint: The precondition only applies to unit test suites. It doesn't apply to DI/Web/Acceptance/Selenium/etc test suites. But, could that be a property on <testsuite>?

[sun]

The manual has been improved for now (not necessarily deployed yet), but regardless of that, this issue kept me thinking recently. As an alternative to my previous comment, please consider this train of thought:

1. ReflectionClass/ReflectionProperty hands us the properties.

2. ReflectionProperty doesn't seem to offer getStartLine() and getEndLine() methods like ReflectionClass, but worst case, export() exposes them.

3. Now use that info to perform the following (various optimizations possible):

   $rc = new ReflectionClass($class);
   $pathname = $rc->getFileName();
   $file = file($pathname);
   $declaration = array_slice($file, $property_start_line, $property_end_line - $property_start_line + 1);
   $tokens = token_get_all('<?php ' . implode("\n", $declaration));
   // ... (reconstruct originally declared default value manually)

The general idea being:

1. Static properties/variables are declarative.
2. Their declaration does not support expressions.
3. All we have to care for are Booleans, NULL, constants, and arrays (whereas arrays may contain scalars, arrays, and aforementioned static types).

In theory, the values can be parsed via Tokenizer once per class, retained in a collection of "clean slate" values, and injected into every "backup/restore" operation.

That operation is still expensive, but compared to HEAD/master, it's a wash, because HEAD iterates over all declared classes for each test method all over again.

Fundamental difference to HEAD/master: All statics that are not part of $backupStaticAttributesBlacklist can not be primed with custom values anymore; they are reset to originally declared default values, as hard-coded into the class.

Would that be a more feasible option? - If time permits, I'll probably try it out in StaticReflection first.

[sebastianbergmann]

I am in the process of factoring out the global state backup/restore code into https://github.com/sebastianbergmann/global-state so I'd rather not make changes to the existing implementation inside PHPUnit.

[RichardBradley]

@sun -- that sounds very complicated and fragile, but I suppose it could work, yes. The saving grace is your point (2): from the docs "Like any other PHP static variable, static properties may only be initialized using a literal or constant; expressions are not allowed". From my experiments, it looks like constant-valued expressions are allowed (e.g. 24 * 60 * 60).

@sebastianbergmann - do you have a timescale for the migration?
(Presumably the functionality hasn't changed, so any fixes here to the static variables backup / restore could be cross-ported easily enough, if necessary.)

[sebastianbergmann]

I hope to be able to make the changes in master later this week.

[sebastianbergmann]

Initial work on this is here: 59ce590

[sebastianbergmann]

Moved to sebastianbergmann/global-state#4.