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

Serialization of 'Closure' is not allowed when using @runInSeparateProcess #4371

Closed
enumag opened this issue Jul 9, 2020 · 15 comments
Closed
Labels
type/bug Something is broken

Comments

@enumag
Copy link

enumag commented Jul 9, 2020

Q A
PHPUnit version 9.2.5
PHP version 7.4.7
Installation Method Composer

Summary

One of our tests started failing with Serialization of 'Closure' is not allowed. When I removed the @runInSeparateProcess (which I have to keep as workaround for https://bugs.php.net/bug.php?id=79646) I got an entirely different error which helped me find the problem in the test.

Apparently there is some issue with process isolation - the data that are serialized to be sent back to the original process contain a Closure and the serialization fails as a result.

Note: This has nothing to do with #2739 - that's about sending data into the test, this is about getting data back.

I'm not sure what data are serialized and where the serialization happens so I'm unable to provide more details and a reproducer for now. Can you point me where in PHPUnit's source code this serialization happens? (Using -v and --debug didn't get me a stack trace.)

@enumag enumag added the type/bug Something is broken label Jul 9, 2020
@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.

@sebastianbergmann sebastianbergmann added the status/waiting-for-feedback Waiting for feedback from original reporter label Jul 9, 2020
@enumag
Copy link
Author

enumag commented Jul 9, 2020

@sebastianbergmann Please read the last paragraph:

I'm not sure what data are serialized and where the serialization happens so I'm unable to provide more details and a reproducer for now. Can you point me where in PHPUnit's source code this serialization happens? (Using -v and --debug didn't get me a stack trace.)

@enumag
Copy link
Author

enumag commented Jul 9, 2020

I need to check the data that are being serialized in order to provide a reproducer. Without knowing where the serialization happens I'm unable to do so.

@mirko-pagliai
Copy link

I have the same problem. And I also don't understand what causes it, what data are serialized and where the serialization happens.

@ramsey
Copy link

ramsey commented Nov 13, 2020

This appears to happen when an assertion called within a Closure fails.

Here's an example that reproduces this:

/**
 * @runInSeparateProcess
 * @preserveGlobalState disabled
 */
public function testSomething(): void
{
    $mock = $this->createMock(\DateTime::class);
    $mock->expects($this->once())
        ->method('format')
        ->with($this->callback(function ($arg1): bool {
            $this->assertSame('foo', $arg1);

            return true;
        }));

    $mock->format('bar');
}

When run as-is, this produces the following fatal error:

PHP Fatal error:  Uncaught Exception: Serialization of 'Closure' is not allowed in Standard input code:89
Stack trace:
#0 Standard input code(89): serialize(Array)
#1 Standard input code(122): __phpunit_run_isolated_test()
#2 {main}
  thrown in Standard input code on line 89
Fatal error: Uncaught Exception: Serialization of 'Closure' is not allowed in Standard input code:89
Stack trace:
#0 Standard input code(89): serialize(Array)
#1 Standard input code(122): __phpunit_run_isolated_test()
#2 {main}
  thrown in Standard input code on line 89

If you change the the argument value for $mock->format() to 'foo', then it passes normally. If you leave it as 'bar' but remove the annotations, you see:

Expectation failed for method name is "format" when invoked 1 time(s)
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'foo'
+'bar'

@Zrnik
Copy link

Zrnik commented Dec 2, 2020

Hello,

i can confirm it has something to do with

This appears to happen when an assertion called within a Closure fails.

I would extend it to 'when assertion is called outside of the test function.

For example:

<?php declare(strict_types=1);

class ExampleTest extends TestCase
{
    /**
     * @runInSeparateProcess
     */
    public function testLanguage(): void
    {
        $languageSwitch = Something::something();

        $this->switchLocale($languageSwitch, "en");

        // ... test something with english ...

        $this->switchLocale($languageSwitch, "de");

        // ... test something with german ...

    }

    private function switchLocale($languageSwitch, $language)
    {
        $languageSwitch->setLanguage($language);

        // This next assert is throwing the error
        // if you comment it, it works fine

        // Edit: only if the assertion fails.

        $this->assertSame(
            $language,
            $languageSwitch->getLanguage()
        );

    }
}

@sebastianbergmann sebastianbergmann removed the status/waiting-for-feedback Waiting for feedback from original reporter label Dec 28, 2020
@sebastianbergmann
Copy link
Owner

Do not call assertions in closures then, sorry.

@Kingdutch
Copy link

This has been occasionally plaguing me for a few months now so I spent a few hours to dig deeper.

Do not call assertions in closures then, sorry.

While I understand the sentiment, that scope for the bug is a bit too small, as illustrated by Zrniks's example and my own tests.

In my investigation I was able to see a difference between self::assertEquals([], $expected); and self::assertEmpty($expected). The former did cause the issue, the latter did not. (Using assertEmpty is not always a workaround in case a comparison needs to be made with a non-empty array, this was just my specific investigation case.)

I was able to figure out that the closure serialisation occurred in the comparisonFailure argument to the ExpectationFailedException instance that's been created on line 109 in the IsEqual comparator. Removing the second parameter fixes the issue but obviously removes a lot of valuable debug information.

By modifying TestCaseMethod.tpl.dist I was able to narrow it down further to the following part of the TestResult: $result->failures()[0]->thrownException().

That is an exception from the test case. Looking further I found that it's backtrace contained a reference to a different error handler that contained itself an error with a stacktrace of a function stack call that contained a closure.

So the error occurs "when any part of your application that produces an error contains a function in the stacktrace that has a closure as one of its arguments".

In my specific case the serialization error was caused in an error with a callstack that contained a call to Visitor::visit that takes a closure.

Although not directly useful, to illustrate the difficulty of debugging this, the full path from TestResult to offending array entry was as follows: $result->failures()[0]->thrownException()[4]['args'][0]->errors[0]->getTrace()[3]['args'][1] where $result is a TestResult instance in __phpunit_run_isolated_test.

Kingdutch added a commit to Kingdutch/graphql that referenced this issue Feb 3, 2021
This commit changes some loops into built-in PHP functions to simplify
the code and eliminate the need for closures.

Additionally this switches over to an `assertEmpty` from `assertEquals`.
This achieves the same test result and in the message still provides the
errors. However it prevents test failures with weird serialization
complaints that are caused by closures in function arguments of GraphQL
Error stacktraces. See sebastianbergmann/phpunit#4371 (comment)

The downside to this change is that we may lose some contextual
information that the full error in the array comparison may provide. The
upside is that we can actually see why tests fail since in a lot of
cases this beneficial diff doesn't make it into the test output anyway.
So all-in-all this is a step forward.
Kingdutch added a commit to Kingdutch/graphql that referenced this issue Feb 3, 2021
This commit changes some loops into built-in PHP functions to simplify
the code and eliminate the need for closures.

Additionally this switches over to an `assertEmpty` from `assertEquals`.
This achieves the same test result and in the message still provides the
errors. However it prevents test failures with weird serialization
complaints that are caused by closures in function arguments of GraphQL
Error stacktraces. See sebastianbergmann/phpunit#4371 (comment)

The downside to this change is that we may lose some contextual
information that the full error in the array comparison may provide. The
upside is that we can actually see why tests fail since in a lot of
cases this beneficial diff doesn't make it into the test output anyway.
So all-in-all this is a step forward.
Kingdutch added a commit to Kingdutch/graphql that referenced this issue Feb 3, 2021
This commit changes some loops into built-in PHP functions to simplify
the code and eliminate the need for closures.

Additionally this switches over to an `assertEmpty` from `assertEquals`.
This achieves the same test result and in the message still provides the
errors. However it prevents test failures with weird serialization
complaints that are caused by closures in function arguments of GraphQL
Error stacktraces. See sebastianbergmann/phpunit#4371 (comment)

The downside to this change is that we may lose some contextual
information that the full error in the array comparison may provide. The
upside is that we can actually see why tests fail since in a lot of
cases this beneficial diff doesn't make it into the test output anyway.
So all-in-all this is a step forward.
klausi pushed a commit to drupal-graphql/graphql that referenced this issue Feb 16, 2021
…ization (#1159)

This commit changes some loops into built-in PHP functions to simplify
the code and eliminate the need for closures.

Additionally this switches over to an `assertEmpty` from `assertEquals`.
This achieves the same test result and in the message still provides the
errors. However it prevents test failures with weird serialization
complaints that are caused by closures in function arguments of GraphQL
Error stacktraces. See sebastianbergmann/phpunit#4371 (comment)

The downside to this change is that we may lose some contextual
information that the full error in the array comparison may provide. The
upside is that we can actually see why tests fail since in a lot of
cases this beneficial diff doesn't make it into the test output anyway.
So all-in-all this is a step forward.
@weierophinney
Copy link

I'm going to add a little more context, as I've just run across the same issue, and have no idea how to change my test to work.

The basic gist of it is:

/**
 * @runInSeparateProcess
 * @preserveGlobalState disabled
 * @dataProvider somethingProvidingAStringMessage
 */
public function testMockingWithConstraints(string $message): void
{
    $mock = $this->createMock(AClassDefiningWriteLine::class);
    $mock
        ->expects($this->once())
        ->method('writeLine')
        ->with($this->stringContains($message));

    $mock->writeLine('<info>' . $message . '</info>');
}

The problem with this statement, @sebastianbergmann :

Do not call assertions in closures then, sorry.

is that the above example demonstrates a documented usage of mock objects, namely, using one of the various constraint methods to provide an assertion to the mock object. But it does not work in this scenario, due to the serialization issues.

I think the issue is valid and should be re-opened.

@weierophinney
Copy link

Another interesting one: if you use withConsecutive(), and any of the arguments fails to match (even if they are not wrapped in a constraint), you also get the serialization error:

$mock
    ->expects($this->exactly(2))
    ->method('writeLine')
    ->withConsecutive(
        ['First message'],
        ['SECOND message'],
    );

$mock->writeLine('First message');
$mock->writeLine('Second message'); // This line triggers the serialization error

@mpyw
Copy link

mpyw commented Jun 23, 2021

I wrote the patch: mpyw/phpunit-patch-serializable-comparison

@aaajeetee
Copy link

I wrote the patch: mpyw/phpunit-patch-serializable-comparison

Can confirm, works! Very nice, thank you!

@DennisdeBest
Copy link

I wrote the patch: mpyw/phpunit-patch-serializable-comparison

Awesome that works for me ! Thanks !

@vasilake-v
Copy link

I wrote the patch: mpyw/phpunit-patch-serializable-comparison

The issue I had was also fixed by this patch. Although, i had different case.


One more case and scenario that i want to share, happening on PHPUnit 9.5.13

The test case itself didn't contain any closures.
But the other side of assertion, the tested implementation had Closures (which i wasn't able to locate)

// \Monolog\Logger , v1.26.1 (2021-05-28)
$loggerMock = $this->getMockBuilder(Logger::class)
	->disableOriginalConstructor()
	->getMock();

$loggerMock->expects($this->once())
	->method('error')
	->with(
		'Failed myControllerActionMethod: error or rejection',
		[
			'item_ids' => [ 111, 222 ],  // <-- this was the difference. The "actual" value was 'item_ids' => []
			'error_code' => 'get_payment_pay_process',
			'customer_id' => '1',
		]
	);

.
.
. After some trial&error I found that the assertion could work without the above package, but the solution doesn't really look so compact.
(Which works by the way without error)

$loggerMock = $this->getMockBuilder(Logger::class)
	->disableOriginalConstructor()
	->getMock();

$loggerMock->expects($this->once())
	->method('error')
	->with(
		'Failed myControllerActionMethod: error or rejection',
		$this->callback(function($args) {
			if ($args['item_ids'] !== [ 111, 222 ]) {
				return false;
			}
			if ($args['error_code'] !== 'get_payment_pay_process') {
				return false;
			}
			if ($args['customer_id'] !== '1') {
				return false;
			}

			return true;
		]
	);

klausi pushed a commit to klausi/graphql that referenced this issue Sep 21, 2023
…ization (drupal-graphql#1159)

This commit changes some loops into built-in PHP functions to simplify
the code and eliminate the need for closures.

Additionally this switches over to an `assertEmpty` from `assertEquals`.
This achieves the same test result and in the message still provides the
errors. However it prevents test failures with weird serialization
complaints that are caused by closures in function arguments of GraphQL
Error stacktraces. See sebastianbergmann/phpunit#4371 (comment)

The downside to this change is that we may lose some contextual
information that the full error in the array comparison may provide. The
upside is that we can actually see why tests fail since in a lot of
cases this beneficial diff doesn't make it into the test output anyway.
So all-in-all this is a step forward.
@Chi-teck
Copy link

Note that some examples do not explicitly use closures. I think those are connected to exception backtraces. See mockery/mockery#1038 (comment)

I wonder if phpunit could handle this in default configuration file.

<ini name="zend.exception_ignore_args" value="On"/>

Danny7007 added a commit to Danny7007/drupal-graphql that referenced this issue Mar 29, 2024
…ization (#1159)

This commit changes some loops into built-in PHP functions to simplify
the code and eliminate the need for closures.

Additionally this switches over to an `assertEmpty` from `assertEquals`.
This achieves the same test result and in the message still provides the
errors. However it prevents test failures with weird serialization
complaints that are caused by closures in function arguments of GraphQL
Error stacktraces. See sebastianbergmann/phpunit#4371 (comment)

The downside to this change is that we may lose some contextual
information that the full error in the array comparison may provide. The
upside is that we can actually see why tests fail since in a lot of
cases this beneficial diff doesn't make it into the test output anyway.
So all-in-all this is a step forward.
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