Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing initialization of setRunClassInSeparateProcess() for tests without data providers #2724

Closed
KintradimCrux opened this issue Jul 3, 2017 · 10 comments
Labels
type/bug Something is broken

Comments

@KintradimCrux
Copy link

Q A
PHPUnit version 6.2.2
PHP version 7.1.6
Installation Method Composer

In the current 6.2.2 release the method TestSuite::createTest() only calls setRunClassInSeparateProcess() for tests with a data provider. Can you add this initialization for tests without a data provider, too?

@sebastianbergmann
Copy link
Owner

@ArturGoldyn ping

@sebastianbergmann sebastianbergmann added the type/bug Something is broken label Jul 3, 2017
@b00gizm
Copy link

b00gizm commented Oct 13, 2017

@KintradimCrux Could you please provide some sample code to reproduce this issue?

@sebastianbergmann sebastianbergmann added the status/waiting-for-feedback Waiting for feedback from original reporter label Oct 13, 2017
@KintradimCrux
Copy link
Author

KintradimCrux commented Oct 15, 2017

@b00gizm Sure.

First, the issue is also present in PHPUnit 6.2.3.

To detect the presence of the isolated environment, i use an autoload.php which sets an environment variable containing the main process PID, if it does not exist yet.

autoload.php:

<?php
$loader = require __DIR__.'/vendor/autoload.php';
if (false === getenv('_MAINPID')) {
    putenv('_MAINPID=' . posix_getpid());
}
return $loader;

RunClassInSeparateProcessTest.php:

<?php
use PHPUnit\Framework\TestCase;
/**
 * @runClassInSeparateProcess
 */
class RunClassInSeparateProcessTest extends TestCase {
    public function __construct($name = null, $data = [], $dataName = ''){
        parent::__construct(null !== $name ? $name : 'test_with_dataProvider', $data, $dataName);
    }
    public function test_without_dataProvider(){
        // This assertion fails
        $this->assertNotEquals((int) getenv('_MAINPID'), posix_getpid());
    }
    public function nullDataProvider(){
        return array(array(null, null, null));
    }
    /**
     * @dataProvider nullDataProvider
     */
    public function test_with_dataProvider(){
        // This assertion does not fail
        $this->assertNotEquals((int) getenv('_MAINPID'), posix_getpid());
    }
}

The overridden constructor is only there to avoid another problem i found while trying to reproduce the issue with this short script. The template in vendor/phpunit/phpunit/src/Util/PHP/Template/TestCaseClass.tpl.dist is creating an instance of the test class with a NULL name instead of using TestSuite for that. The result of this is an Exception with message "PHPUnit\Framework\TestCase::$name must not be null" when the runTest() method of the test class is called. See Issue #2830

Method PHPUnit\Framework\TestSuite::createTest() contains a code block for test cases with a data provider, which calls the setRunClassInSeparateProcess() method of the test:

                // Test method with @dataProvider.
                if (isset($data)) {
// ...
                            if ($runClassInSeparateProcess) {
                                $_test->setRunClassInSeparateProcess(true);
                                if ($preserveGlobalState !== null) {
                                    $_test->setPreserveGlobalState($preserveGlobalState);
                                }
                            }
// ...
                } else {
                    $test = new $className;
                }

At the end of the createTest() method theres a similar code block for tests without a data provider, which does not call setRunClassInSeparateProcess():

        if ($test instanceof TestCase) {
            $test->setName($name);
            if ($runTestInSeparateProcess) {
                $test->setRunTestInSeparateProcess(true);
                if ($preserveGlobalState !== null) {
                    $test->setPreserveGlobalState($preserveGlobalState);
                }
            }
            if ($backupSettings['backupGlobals'] !== null) {
                $test->setBackupGlobals($backupSettings['backupGlobals']);
            }
            if ($backupSettings['backupStaticAttributes'] !== null) {
                $test->setBackupStaticAttributes(
                    $backupSettings['backupStaticAttributes']
                );
            }
        }

@sebastianbergmann
Copy link
Owner

Thank you for your report.

Please provide a minimal, self-contained, reproducing test case that shows the problem you are reporting.

Without such a minimal, self-contained, reproducing test case I will not be able to investigate this issue.

@KintradimCrux
Copy link
Author

KintradimCrux commented Nov 28, 2017

@sebastianbergmann Extract the attached ZIP issue2724.zip or create a directory containing the following files:

composer.json:

{
	"require": {
		"phpunit/phpunit": "6.2.3"
	},
	"autoload": {
		"psr-4": {
			"": "tests/"
		}
	}
}

autoload.php:

<?php
$loader = require __DIR__.'/vendor/autoload.php';
if (false === getenv('_MAINPID')) {
    putenv('_MAINPID=' . posix_getpid());
}
return $loader;

phpunit.xml.dist:

<?xml version="1.0" encoding="UTF-8"?>

<!-- https://phpunit.de/manual/current/en/appendixes.configuration.html -->
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:noNamespaceSchemaLocation="http://schema.phpunit.de/4.8/phpunit.xsd"
         backupGlobals="true"
         backupStaticAttributes="true"
         colors="true"
         bootstrap="autoload.php"
>
    <php>
    	<env name="COLUMNS" value="80"/>
    	<env name="LINES" value="50"/>
        <ini name="error_reporting" value="-1" />
    </php>

    <testsuites>
        <testsuite name="Project Test Suite">
            <directory>tests/</directory>
        </testsuite>
    </testsuites>

    <filter>
        <whitelist>
        </whitelist>
    </filter>
</phpunit>

Create the subdirectory "tests/" and put the following files in it:

RunClassInSeparateProcessTest.php:

<?php
use PHPUnit\Framework\TestCase;
/**
 * @runClassInSeparateProcess
 */
class RunClassInSeparateProcessTest extends TestCase {
    public function __construct($name = null, $data = [], $dataName = ''){
        parent::__construct(null !== $name ? $name : 'test_with_dataProvider', $data, $dataName);
    }
    public function test_without_dataProvider(){
        // This assertion fails
        $this->assertNotEquals((int) getenv('_MAINPID'), posix_getpid());
    }
    public function nullDataProvider(){
        return array(array(null, null, null));
    }
    /**
     * @dataProvider nullDataProvider
     */
    public function test_with_dataProvider(){
        // This assertion does not fail
        $this->assertNotEquals((int) getenv('_MAINPID'), posix_getpid());
    }
}

Run "composer install", then run "vendor/bin/phpunit".

When removing the constructor of class RunClassInSeparateProcessTest, you can use the same test case to trigger the incorrect behavior of the isolated environment reported in Issue #2830.

@sebastianbergmann
Copy link
Owner

PHPUnit 6.2.3 is no longer supported. Before I investigate this, please make sure that the issue persists with PHPUnit 6.4.4.

@KintradimCrux
Copy link
Author

@sebastianbergmann Using PHPUnit 6.4.4, the assertion in test_without_dataProvider() still fails because of not being run in a sub-process. I attached a new issue2724.zip with an updated composer.json.

@KintradimCrux
Copy link
Author

KintradimCrux commented Nov 29, 2017

@sebastianbergmann I already fixed the issue in my fork and can do a PR if required, but im not sure where to add that environment variable for the tests.

@KintradimCrux
Copy link
Author

KintradimCrux commented Nov 29, 2017

@sebastianbergmann I just noticed that the change makes some of the tests for Issue #2591 fail with the same message i reported in Issue #2830. This needs to be investigated further first.

Update: turns out the tests for Issue #2591 never were run in isolation due to the missing initialization, do not include tests with a data provider and also aren't testing they actually are running in a subprocess. Without the constructor override in my provided test class both of my test cases run into the same problem. I guess Issue #2830 needs to be fixed first.

KintradimCrux added a commit to KintradimCrux/phpunit that referenced this issue Nov 30, 2017
@sebastianbergmann sebastianbergmann removed the status/waiting-for-feedback Waiting for feedback from original reporter label Apr 7, 2018
@mihailogajic
Copy link
Contributor

mihailogajic commented Apr 7, 2018

PR that contains a proposal for solving this issue #3083

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Something is broken
Projects
None yet
Development

No branches or pull requests

4 participants