When backupStaticAttributes is enable, static data from a first test case leaks to the subsequent tests cases

[ghost]

Basically, when using autoloading, the framework cannot have access to a certain class until it's autoloaded. Then, if the first test case stored something in the static scope, it will be available to all the subsequent tests cases and, if someone access the static scope, it can breaks a test with an unexpected behavior. It's an edge case.

I created some example files to show this: http://github.com/eriksencosta/phpunit-bsa-edge-case

Just run AllTests and see that the assertEquals calls fails. Then, uncommenting the 14th line from OneTest will cause the tests passes (or just checkouts the "pass" branch).

One solution I thought was to replace the registered autoloaders with rewrited copies that would call GlobalState::backupStaticAttributes() when the class was first loaded (something like what MockObject do when create the mock with a class skeleton).

For this, backupStaticAttributes could be refactored and a specific method would backup the static attributes of a newly loaded class. All the subsequent calls would behave as expected because as the class name would be returned in the get_declared_classes() call of GlobalState::backupStaticAttributes().

Maybe with the backupStaticAttributes = FALSE recommendation, this could be an unnecessary effort. But while the option exists, some people will eventually have problems with this, maybe thinking is misusing the option.

[sebastianbergmann]

I do not see a bug here.

[RichardBradley]

This is causing me problems as well.
I can workaround the issue by autoloading all affected classes before the test suite runs, but this is annoying and requires me to identify which classes are affected (which is difficult) and list them in my test bootstrap (which is a failure of encapsulation).

I think this is a real bug.

I'm not sure how this could be fixed. 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).

[RichardBradley]

This bug still exists. I think this was closed by mistake due to an unrelated "#1" in the commit auto-linked above. Please could you re-open, to track the issue?

This may be difficult or impossible to fix, but it should still be tracked :-)

[sun]

@RichardBradley The example code repo from the OP no longer exists. Are you able to provide a new isolated test case?

[RichardBradley]

OK, will do

[thehereward]

I have created a repo with a reproduction case here: https://github.com/thehereward/phpunit-backup-static-attributes-bug

[RichardBradley]

To summarise the bug:

• 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 in many cases, 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.

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.

It is not clear to me that this is easily fixable, but it is a real bug.
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.

Please could you reopen the issue?
Do you have any idea how this might be fixed?

[RichardBradley]

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.

[sun]

@RichardBradley Thanks a lot for the elaborate description and thanks @thehereward for redoing an isolated test case. I understood the problem now, and I think I also noticed that behavior already.

Especially with autoloaded classes, statics in newly loaded classes will not be reset, and worse, the list of statics to backup increases over time (because a new static of the previous test becomes a preexisting static to backup in all subsequent tests).

I think that's worth to investigate and fix. However, due to various issue references (unintentionally unquoted test data provider #1 as well as PHP stack trace frame #1), this issue continues to get automatically closed by GitHub. Can we move this into a new issue, using the last comments as a good, initial summary?

[RichardBradley]

OK, now recreated at #1372