Closed rlerdorf closed 5 years ago
Ok, tracked it down to the latest commit: 40301f79ac47624d062f86e4ea54ad0664b3b519
I was sure I'd be able to reproduce if I covered all paths for cufa, but I don't seem able to reproduce the fault having tested all paths. Still I think I spotted a bit of inconsistency there and fixed that ... can you try HEAD and see ?
I might need a reproducing script, if that didn't get it ... one with less than 56k frames, I hope :D
segfault is still there. I will try to come up with a reproduce case, but it is deep in some complicated phpunit tests. It looks like some eval'ed code ends up going into an infinite recursive loop.
That eval is a nasty hack to try to emulate the way test_helpers used to overload just the new
operator allowing static method calls and constants to hit the original class. Unfortunately a bunch of tests rely on that in this project, so when replacing test_helpers with uopz, that is a huge hassle.
I await reproducing code ... am all out of ideas ...
This might be a useful pattern ?
<?php
class Target {
const CVAL = 10;
public static function method() {
return self::mocked();
}
public static function mocked() {
return false;
}
}
class MockTarget extends Target {
public static function mocked() {
return true;
}
}
uopz_set_mock(Target::class, MockTarget::class);
$target = new Target();
var_dump($target); // object(MockTarget)
var_dump(Target::method()); // bool(true)
var_dump(Target::CVAL); // int(10)
?>
Yeah, we tried that pattern with extending the original class, but hit some issues. @ericnorris do you remember what the issue was there?
All of the handlers to make this behave consistently have been committed since the last release, it's probably worth another try.
@rlerdorf yeah, the build of uopz
a week or two ago would:
a) not respect the $flags
parameter in uopz_add_function
b) segfault when calling the uopz_flag function when setting flags as a workaround to a)
I'll see if I can try the latest uopz
release and check if it's still happening.
@rlerdorf I've tested the following code against uopz-5.1.0
, although I don't have access to the Etsy testing code so the results might not be the same:
<?php declare(strict_types=1);
require_once '../vendor/autoload.php';
use PHPUnit\Framework\TestCase;
class A {
public static function publicFoo() {
return A::privateFoo();
}
private static function privateFoo() {
return 'hello from A';
}
}
class ATest extends TestCase {
public function test_naive() {
$mock_object = $this
->getMockBuilder(A::class)
->getMock();
uopz_set_mock(A::class, $mock_object);
// fails because the PHPUnit mock intercepts the static call
$this->assertSame('hello from A', A::publicFoo());
}
public function test_uopz_set_return() {
$original_foo_method = (new ReflectionMethod(A::class, 'publicFoo'))
->getClosure()
->bindTo(null, A::class);
$mock_object = $this
->getMockBuilder(A::class)
->getMock();
$mock_class = get_class($mock_object);
// used to fail here, because uopz_set_return would not handle
// 'publicFoo' being on a parent, could be Etsy testing weirdness
uopz_set_return($mock_class, 'publicFoo', $original_foo_method, true);
uopz_set_mock(A::class, $mock_object);
$this->assertSame('hello from A', A::publicFoo());
}
public function test_uopz_set_return_nested_closure() {
$original_foo_method = (new ReflectionMethod(A::class, 'publicFoo'))
->getClosure()
->bindTo(null, A::class);
$nested_closure =
function(...$args) use ($original_foo_method) {
return $original_foo_method(...$args);
};
$mock_object = $this
->getMockBuilder(A::class)
->getMock();
$mock_class = get_class($mock_object);
uopz_set_return($mock_class, 'publicFoo', $nested_closure, true);
uopz_set_mock(A::class, $mock_object);
// works!
$this->assertSame('hello from A', A::publicFoo());
}
public function test_proxy_class_preserve_uopz_flags() {
$original_class = new ReflectionClass(A::class);
$original_statics = $original_class->getMethods(
ReflectionMethod::IS_STATIC
);
$proxy_class_name = A::class . "_uopz_proxy";
eval("class $proxy_class_name {}");
foreach ($original_statics as $static_method) {
$flags = uopz_flags(
$static_method->getDeclaringClass()->getName(),
$static_method->getName()
);
$closure = $static_method->getClosure();
uopz_add_function(
$proxy_class_name,
$static_method->getName(),
$closure,
$flags
);
}
$mock_object = $this
->getMockBuilder(A::class)
->getMock();
$mock_class = get_class($mock_object);
uopz_extend($proxy_class_name, $mock_class);
uopz_set_mock(A::class, $proxy_class_name);
$this->assertSame('hello from A', A::publicFoo());
}
public function test_proxy_class_explicit_uopz_flags() {
$original_class = new ReflectionClass(A::class);
$original_statics = $original_class->getMethods(
ReflectionMethod::IS_STATIC
);
$proxy_class_name = A::class . "_uopz_proxy";
eval("class $proxy_class_name {}");
foreach ($original_statics as $static_method) {
$flags = uopz_flags(
$static_method->getDeclaringClass()->getName(),
$static_method->getName()
);
$closure = $static_method->getClosure();
uopz_add_function(
$proxy_class_name,
$static_method->getName(),
$closure
);
uopz_flags($proxy_class_name, $static_method->getName(), $flags);
}
$mock_object = $this
->getMockBuilder(A::class)
->getMock();
$mock_class = get_class($mock_object);
uopz_extend($proxy_class_name, $mock_class);
uopz_set_mock(A::class, $proxy_class_name);
$this->assertSame('hello from A', A::publicFoo());
}
public function test_proxy_class_explicit_flags_add_function() {
$original_class = new ReflectionClass(A::class);
$original_statics = $original_class->getMethods(
ReflectionMethod::IS_STATIC
);
$proxy_class_name = A::class . "_uopz_proxy";
eval("class $proxy_class_name {}");
foreach ($original_statics as $static_method) {
$closure = $static_method->getClosure();
$flags = ZEND_ACC_PUBLIC | ZEND_ACC_STATIC;
if ($static_method->isPrivate()) {
$flags = ZEND_ACC_PRIVATE | ZEND_ACC_STATIC;
}
uopz_add_function(
$proxy_class_name,
$static_method->getName(),
$closure,
$flags
);
}
$mock_object = $this
->getMockBuilder(A::class)
->getMock();
$mock_class = get_class($mock_object);
uopz_extend($proxy_class_name, $mock_class);
uopz_set_mock(A::class, $proxy_class_name);
$this->assertSame('hello from A', A::publicFoo());
}
}
The output I get:
docker run -it --rm php7.3-uopz-5.1.0
PHPUnit 7.5.1 by Sebastian Bergmann and contributors.
WE.E.E 6 / 6 (100%)
Time: 124 ms, Memory: 4.00MB
There were 3 errors:
1) ATest::test_uopz_set_return
Error: Call to private method A::privateFoo() from context 'Mock_A_9bf8dc7d'
/atest.php:10
/atest.php:47
2) ATest::test_proxy_class_preserve_uopz_flags
PHPUnit\Framework\Exception: Fatal error: Access level to A_uopz_proxy::privateFoo() must be private (as in class A) or weaker in /atest.php on line 105
3) ATest::test_proxy_class_explicit_flags_add_function
PHPUnit\Framework\Exception: Fatal error: Access level to A_uopz_proxy::privateFoo() must be private (as in class A) or weaker in /atest.php on line 182
--
There was 1 warning:
1) ATest::test_naive
Static method "publicFoo" cannot be invoked on mock object
ERRORS!
Tests: 6, Assertions: 2, Errors: 3, Warnings: 1.
make: *** [makefile:12: run] Error 2
Extending the class directly (test_naive
) does not work, as that is handled by PHPUnit; PHPUnit unfortunately makes stubs for all the static methods and will generate a warning when called.
Using uopz_set_return
did not work on older versions of uopz
and in the Etsy testing environment, it seems that here it does work so long as you do some dancing around the closure you pass in (test_uopz_set_return_nested_closure
).
Creating a proxy and extending the class after creation does not work as uopz
apparently mishandles the Zend flags which causes the access level to not match (test_proxy_class_preserve_uopz_flags
and test_proxy_class_explicit_flags_add_function
). Using a proxy class was only a workaround for uopz_set_return
not working on a parent method in a child class, however, so it might not be needed.
I can test this on Monday, and if the latest version of uopz
doesn't solve the need for nasty eval
workaround I'll see if I can get a smaller reproducing segfault.
Extending the class directly (test_naive) does not work, as that is handled by PHPUnit; PHPUnit unfortunately makes stubs for all the static methods and will generate a warning when called.
If you're using some API from phpunit mock builder then what I'm about to say is probably not very useful ... but if you're not, and you're just using the mock builder to stub/mock functions, then you don't need the phpunit mock:
<?php declare(strict_types=1);
require_once 'vendor/autoload.php';
use PHPUnit\Framework\TestCase;
class A {
public static function publicStaticFoo() {
return A::privateStaticFoo();
}
private static function privateStaticFoo() {
return 'static hello from A';
}
public function publicFoo() {
return $this->privateFoo();
}
private function privateFoo() {
return 'hello from A';
}
}
class ATest extends TestCase {
public function testStubbingPublicStatic() {
$mock = new class extends A {
static function publicStaticFoo() {
return 'hello from uopz';
}
};
uopz_set_mock(A::class, $mock);
$this->assertSame('hello from uopz', A::publicStaticFoo());
uopz_unset_mock(A::class);
}
public function testStubbingPrivateStatic() {
$mock = new class extends A {
static function privateStaticFoo() {
return 'hello from uopz';
}
};
uopz_set_mock(A::class, $mock);
$this->assertSame('hello from uopz', A::publicStaticFoo());
uopz_unset_mock(A::class);
}
public function testStubbingPublic() {
$mock = new class extends A {
function publicFoo() {
return 'hello from uopz';
}
};
uopz_set_mock(A::class, $mock);
$object = new A();
$this->assertSame('hello from uopz', $object->publicFoo());
uopz_unset_mock(A::class);
}
public function testStubbingPrivate() {
uopz_flags(A::class, "privateFoo", ZEND_ACC_PROTECTED);
$mock = new class extends A {
function privateFoo() {
return 'hello from uopz';
}
};
uopz_flags(A::class, "privateFoo", ZEND_ACC_PRIVATE);
uopz_set_mock(A::class, $mock);
$object = new A();
$this->assertSame('hello from uopz', $object->publicFoo());
uopz_unset_mock(A::class);
}
}
That's how I would do it ... you have to jump through hoops still for mocking private non static methods, but to my eyes it looks less nasty than the code you had ...
I just thought I would mention, because it might be useful ...
uopz_set_mock
is a powerful API because it can affect instances of objects that were constructed before the call to uopz_set_mock
is made and without access to the object, which is sometimes necessary ... it does this by overloading the zend engine, quite heavily. That's acceptable because uopz is focused on aiding unit testing, but it doesn't actually provide an API for composing classes, so it's a bit awkward because it uses the class or object you provide verbatim (and it has normal inherited ze semantics)
Componere is an extension to (re)compose classes at runtime, it doesn't overload the engine, it has a class building API: Definition. It also has an API for affecting instances of objects Patch where you have access to the object. The classes it builds will reflect as if you had written them as modified. The following is the same test code as above, but totally without uopz ...
<?php declare(strict_types=1);
require_once 'vendor/autoload.php';
use PHPUnit\Framework\TestCase;
use Componere\Definition;
use Componere\Method;
class A {
public static function publicStaticFoo() {
return A::privateStaticFoo();
}
private static function privateStaticFoo() {
return 'static hello from A';
}
public function publicFoo() {
return $this->privateFoo();
}
private function privateFoo() {
return 'hello from A';
}
}
class ATest extends TestCase {
public function testStubbingPublicStatic() {
$definition = new Definition(A::class);
$method = new Method(function(){
return 'Componere says hi!';
});
$method->setStatic();
$definition->addMethod("publicStaticFoo", $method);
$definition->register();
$this->assertSame('Componere says hi!', A::publicStaticFoo());
}
public function testStubbingPrivateStatic() {
$definition = new Definition(A::class);
$method = new Method(function(){
return 'Componere says hi!';
});
$method->setPrivate();
$method->setStatic();
$definition->addMethod("privateStaticFoo", $method);
$definition->register();
$this->assertSame('Componere says hi!', A::publicStaticFoo());
}
public function testStubbingPublic() {
$definition = new Definition(A::class);
$method = new Method(function(){
return 'Componere says hi!';
});
$definition->addMethod("publicFoo", $method);
$definition->register();
$object = new A();
$this->assertSame('Componere says hi!', $object->publicFoo());
}
public function testStubbingPrivate() {
$definition = new Definition(A::class);
$method = new Method(function(){
return 'Componere says hi!';
});
$method->setPrivate();
$definition->addMethod("privateFoo", $method);
$definition->register();
$object = new A();
$this->assertSame('Componere says hi!', $object->publicFoo());
}
}
I hope that's useful to know about ...
None of this solves the fault, which I'm still interested in reproducing ...
It may be worth another test with HEAD ...
The segfault appears to be me blowing the stack in a tight recursion loop. So at least this particular one doesn't appear to be uopz's fault.
@rlerdorf I've tested the following code against
uopz-5.1.0
, although I don't have access to the Etsy testing code so the results might not be the same:<?php declare(strict_types=1); require_once '../vendor/autoload.php'; use PHPUnit\Framework\TestCase; class A { public static function publicFoo() { return A::privateFoo(); } private static function privateFoo() { return 'hello from A'; } } class ATest extends TestCase { public function test_naive() { $mock_object = $this ->getMockBuilder(A::class) ->getMock(); uopz_set_mock(A::class, $mock_object); // fails because the PHPUnit mock intercepts the static call $this->assertSame('hello from A', A::publicFoo()); } public function test_uopz_set_return() { $original_foo_method = (new ReflectionMethod(A::class, 'publicFoo')) ->getClosure() ->bindTo(null, A::class); $mock_object = $this ->getMockBuilder(A::class) ->getMock(); $mock_class = get_class($mock_object); // used to fail here, because uopz_set_return would not handle // 'publicFoo' being on a parent, could be Etsy testing weirdness uopz_set_return($mock_class, 'publicFoo', $original_foo_method, true); uopz_set_mock(A::class, $mock_object); $this->assertSame('hello from A', A::publicFoo()); } public function test_uopz_set_return_nested_closure() { $original_foo_method = (new ReflectionMethod(A::class, 'publicFoo')) ->getClosure() ->bindTo(null, A::class); $nested_closure = function(...$args) use ($original_foo_method) { return $original_foo_method(...$args); }; $mock_object = $this ->getMockBuilder(A::class) ->getMock(); $mock_class = get_class($mock_object); uopz_set_return($mock_class, 'publicFoo', $nested_closure, true); uopz_set_mock(A::class, $mock_object); // works! $this->assertSame('hello from A', A::publicFoo()); } public function test_proxy_class_preserve_uopz_flags() { $original_class = new ReflectionClass(A::class); $original_statics = $original_class->getMethods( ReflectionMethod::IS_STATIC ); $proxy_class_name = A::class . "_uopz_proxy"; eval("class $proxy_class_name {}"); foreach ($original_statics as $static_method) { $flags = uopz_flags( $static_method->getDeclaringClass()->getName(), $static_method->getName() ); $closure = $static_method->getClosure(); uopz_add_function( $proxy_class_name, $static_method->getName(), $closure, $flags ); } $mock_object = $this ->getMockBuilder(A::class) ->getMock(); $mock_class = get_class($mock_object); uopz_extend($proxy_class_name, $mock_class); uopz_set_mock(A::class, $proxy_class_name); $this->assertSame('hello from A', A::publicFoo()); } public function test_proxy_class_explicit_uopz_flags() { $original_class = new ReflectionClass(A::class); $original_statics = $original_class->getMethods( ReflectionMethod::IS_STATIC ); $proxy_class_name = A::class . "_uopz_proxy"; eval("class $proxy_class_name {}"); foreach ($original_statics as $static_method) { $flags = uopz_flags( $static_method->getDeclaringClass()->getName(), $static_method->getName() ); $closure = $static_method->getClosure(); uopz_add_function( $proxy_class_name, $static_method->getName(), $closure ); uopz_flags($proxy_class_name, $static_method->getName(), $flags); } $mock_object = $this ->getMockBuilder(A::class) ->getMock(); $mock_class = get_class($mock_object); uopz_extend($proxy_class_name, $mock_class); uopz_set_mock(A::class, $proxy_class_name); $this->assertSame('hello from A', A::publicFoo()); } public function test_proxy_class_explicit_flags_add_function() { $original_class = new ReflectionClass(A::class); $original_statics = $original_class->getMethods( ReflectionMethod::IS_STATIC ); $proxy_class_name = A::class . "_uopz_proxy"; eval("class $proxy_class_name {}"); foreach ($original_statics as $static_method) { $closure = $static_method->getClosure(); $flags = ZEND_ACC_PUBLIC | ZEND_ACC_STATIC; if ($static_method->isPrivate()) { $flags = ZEND_ACC_PRIVATE | ZEND_ACC_STATIC; } uopz_add_function( $proxy_class_name, $static_method->getName(), $closure, $flags ); } $mock_object = $this ->getMockBuilder(A::class) ->getMock(); $mock_class = get_class($mock_object); uopz_extend($proxy_class_name, $mock_class); uopz_set_mock(A::class, $proxy_class_name); $this->assertSame('hello from A', A::publicFoo()); } }
The output I get:
docker run -it --rm php7.3-uopz-5.1.0 PHPUnit 7.5.1 by Sebastian Bergmann and contributors. WE.E.E 6 / 6 (100%) Time: 124 ms, Memory: 4.00MB There were 3 errors: 1) ATest::test_uopz_set_return Error: Call to private method A::privateFoo() from context 'Mock_A_9bf8dc7d' /atest.php:10 /atest.php:47 2) ATest::test_proxy_class_preserve_uopz_flags PHPUnit\Framework\Exception: Fatal error: Access level to A_uopz_proxy::privateFoo() must be private (as in class A) or weaker in /atest.php on line 105 3) ATest::test_proxy_class_explicit_flags_add_function PHPUnit\Framework\Exception: Fatal error: Access level to A_uopz_proxy::privateFoo() must be private (as in class A) or weaker in /atest.php on line 182 -- There was 1 warning: 1) ATest::test_naive Static method "publicFoo" cannot be invoked on mock object ERRORS! Tests: 6, Assertions: 2, Errors: 3, Warnings: 1. make: *** [makefile:12: run] Error 2
Extending the class directly (
test_naive
) does not work, as that is handled by PHPUnit; PHPUnit unfortunately makes stubs for all the static methods and will generate a warning when called.Using
uopz_set_return
did not work on older versions ofuopz
and in the Etsy testing environment, it seems that here it does work so long as you do some dancing around the closure you pass in (test_uopz_set_return_nested_closure
).Creating a proxy and extending the class after creation does not work as
uopz
apparently mishandles the Zend flags which causes the access level to not match (test_proxy_class_preserve_uopz_flags
andtest_proxy_class_explicit_flags_add_function
). Using a proxy class was only a workaround foruopz_set_return
not working on a parent method in a child class, however, so it might not be needed.I can test this on Monday, and if the latest version of
uopz
doesn't solve the need for nastyeval
workaround I'll see if I can get a smaller reproducing segfault.
This original code will run as expected with d8ce60f39f44f51c6252d89149ac3ec3765b9afa
krakjoe@fiji:/opt/src/uopz$ vendor/bin/phpunit etsy.php --verbose --process-isolation
PHPUnit 7.5.1 by Sebastian Bergmann and contributors.
Runtime: PHP 7.3.3-dev
...... 6 / 6 (100%)
Time: 236 ms, Memory: 4.00MB
OK (6 tests, 6 assertions)
I think you're good to go now ...
Something changed between commits 9608e76bc8edee31c5a7d4083aae1448bf75368a and 40301f79ac47624d062f86e4ea54ad0664b3b519 that is causing a segfault running some phpunit tests under PHP 7.3. I haven't narrowed it down yet, but I will try. The gdb backtrace is an impressive 56532 frames deep. The final dozen frames look like this: