1) Drupal\Tests\Core\Update\UpdateRegistryTest::testGetPendingUpdateFunctionsNoExistingUpdates
PHPUnit_Framework_Exception: PHP Notice: Constant REQUIREMENT_INFO already defined in /.../d8/core/includes/install.inc on line 19
PHP Stack trace:
PHP 1. {main}() -:0
PHP 2. require_once() -:1676

Notice: Constant REQUIREMENT_INFO already defined in /.../d8/core/includes/install.inc on line 19

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul created an issue. See original summary.

dawehner’s picture

Really interesting because I certainly used phpunit --filter "UpdateRegistryTest" only.
Why do we even load install.inc twice here, well why even the first time.

neclimdul’s picture

Status: Active » Needs review
FileSize
496 bytes

So that introduced the failure but the bug was in ModuleHandlerTest which includes install.inc. I haven't looked at the phpunit internals but there is probably some form of fork call that keeps brings along global state. so even when you run in a separate process if someone else pollutes the global space you get a failure.

#2105583: Add some sane strictness to phpunit tests to catch risky tests probably would have caught that but I've been bad and hadn't followed through with it.. :(

dawehner’s picture

This fixes the issue, let's get it in.

On the longrun for those tests we should consider converting them to vfs entirely and empty out install.inc basically.

neclimdul’s picture

Sadly my previous statement seems to be false and that wouldn't have caught it. beStrictAboutChangesToGlobalState only captures globals, super globals and static attributes on classes. defining new constants are not caught.

Status: Needs review » Needs work

The last submitted patch, 3: failures_running-2565971-3.patch, failed testing.

neclimdul’s picture

Status: Needs work » Needs review
alexpott’s picture

This breaks my commit workflow... we need to care about the global state. Basically whenever @runTestsInSeparateProcesses is used in conjunction with tests that run in the main process we should also do @preserveGlobalState disabled.

Unit tests that load includes not through the autoloader suck.

Status: Needs review » Needs work

The last submitted patch, 9: 2565971.9.patch, failed testing.

dawehner’s picture

  1. --- a/.htaccess
    +++ b/.htaccess
    
    +++ b/.htaccess
    @@ -123,7 +123,7 @@ AddEncoding gzip svgz
    
    @@ -123,7 +123,7 @@ AddEncoding gzip svgz
       #
       # If your site is running in a VirtualDocumentRoot at http://example.com/,
       # uncomment the following line:
    -  # RewriteBase /
    +  RewriteBase /
     
    

    meh

  2. +++ b/core/tests/Drupal/Tests/Core/Extension/ModuleHandlerTest.php
    index 5de6780..5ef5ecf 100644
    --- a/core/tests/Drupal/Tests/Core/Update/UpdateRegistryTest.php
    
    --- a/core/tests/Drupal/Tests/Core/Update/UpdateRegistryTest.php
    +++ b/core/tests/Drupal/Tests/Core/Update/UpdateRegistryTest.php
    
    +++ b/core/tests/Drupal/Tests/Core/Update/UpdateRegistryTest.php
    +++ b/core/tests/Drupal/Tests/Core/Update/UpdateRegistryTest.php
    @@ -17,9 +17,10 @@
    
    @@ -17,9 +17,10 @@
      * @coversDefaultClass \Drupal\Core\Update\UpdateRegistry
      * @group Update
      *
    - * Note we load code, so we should better run in isolation.
    + * Note we load code, so isolate the tests.
      *
      * @runTestsInSeparateProcesses
    + * @preserveGlobalState disabled
      */
     class UpdateRegistryTest extends UnitTestCase {
     
    

    Do we need that part actually? I thought we don't need the isolation as we use vfs only?

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
  1. alexpott--
  2. Yes we should do this because if we don't all files included in the parent process will be included in the child process. As stated above...

    Basically whenever @runTestsInSeparateProcesses is used in conjunction with tests that run in the main process we should also do @preserveGlobalState disabled.

alexpott’s picture

Oops... third time lucky.

The last submitted patch, 12: 2565971.9.patch, failed testing.

alexpott’s picture

For those wondering why this happens and why #13 is the right fix...

 if ($this->preserveGlobalState) {
                $constants     = PHPUnit_Util_GlobalState::getConstantsAsString();
                $globals       = PHPUnit_Util_GlobalState::getGlobalsAsString();
                $includedFiles = PHPUnit_Util_GlobalState::getIncludedFilesAsString();
                $iniSettings   = PHPUnit_Util_GlobalState::getIniSettingsAsString();

in PHPUnit_Framework_TestCase::run()

The last submitted patch, 9: 2565971.9.patch, failed testing.

The last submitted patch, 12: 2565971.9.patch, failed testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you alex for explaining me what is going on.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

neclimdul’s picture

Status: Fixed » Needs work
+++ b/core/tests/Drupal/Tests/Core/Extension/ModuleHandlerTest.php
@@ -265,6 +265,7 @@ public function testLoadAllIncludes() {
+   * @runInSeparateProcess

This is the correct annotation

+++ b/core/tests/Drupal/Tests/Core/Extension/ModuleHandlerTest.php
@@ -265,6 +265,11 @@ public function testLoadAllIncludes() {
+   * @runTestsInSeparateProcesses

This was the wrong annotation.

Edit: removed stray sentence that was explained in IRC(and above) prior but was accidentally added..

  • catch committed 7aa798f on 8.0.x
    Issue #2565971 by alexpott, neclimdul: failures running phpunit from...
neclimdul’s picture

Status: Needs work » Needs review
FileSize
564 bytes

quick follow up.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

My bad. Sorry @neclimdul

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: failures_running-2565971-21.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

The DrupalCI fail is unrelated.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for catching this @neclimdul. Committed 04b7868 and pushed to 8.0.x. Thanks!

  • alexpott committed 04b7868 on 8.0.x
    Issue #2565971 followup by neclimdul: failures running phpunit from...

Status: Fixed » Closed (fixed)

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