Page MenuHomePhabricator

Can't test handlers with a param of type 'title' with HandlerTestTrait
Closed, ResolvedPublicBUG REPORT

Description

If a REST handler has a title parameter, e.g.

'page' => [
	static::PARAM_SOURCE => 'post',
	ParamValidator::PARAM_TYPE => 'title',
	ParamValidator::PARAM_REQUIRED => true,
],

and you try to test it with HandlerTestTrait, e.g.

$handler = // Instantiate the handler
$reqData = new RequestData( /* Pass data */ );
$this->executeHandlerAndGetBodyData( $handler, $request );

it will fail with

Wikimedia\Services\ServiceContainer::get('TitleFactory') was not expected to be called.

The reason is that HandlerTestTrait::validateHandler has:

$serviceContainer = $this->createNoOpMock( ServiceContainer::class );
$objectFactory = new ObjectFactory( $serviceContainer );
$validator = new Validator( $objectFactory, $handler->getRequest(), $handler->getAuthority() );

And createNoOpMock creates a mock which never expects any method to be called. Note that even if you replace it with createMock it still fails because ServiceContainer::get( 'TitleFactory' ) returns null (since the method has no return type information) and that is passed to TitleDef::__construct. So it needs something like

$serviceContainer = $this->createMock( ServiceContainer::class );
$serviceContainer->method( 'get' )->with( 'TitleFactory' )->willReturn( $this->createMock( TitleFactory::class ) );

but then the mocked TitleFactory's newFromText will return null because the actual method can also return null. So you also need

$serviceContainer = $this->createMock( ServiceContainer::class );
$tf = $this->createMock( TitleFactory::class );
$tf->method( 'newFromText' )->willReturn( $this->createMock( Title::class ) );
$serviceContainer->method( 'get' )->with( 'TitleFactory' )->willReturn( $tf );

which means it's always going to assume that the Title is valid, and you cannot test the case where it isn't. So perhaps it'd be better if HandlerTestTrait allowed callers to inject a validator, and fall back to a mock when it's not provided.

Event Timeline

I just realized that the same is true for parameters of type 'user'. In that case, the stack trace will be something like:

Wikimedia\Services\ServiceContainer::get('UserIdentityLookup') was not expected to be called.

/workspace/src/vendor/wikimedia/object-factory/src/ObjectFactory.php:211
/workspace/src/vendor/wikimedia/object-factory/src/ObjectFactory.php:152
/workspace/src/includes/libs/ParamValidator/ParamValidator.php:327
/workspace/src/includes/libs/ParamValidator/ParamValidator.php:366
/workspace/src/includes/libs/ParamValidator/ParamValidator.php:516
/workspace/src/includes/Rest/Validator/Validator.php:108
/workspace/src/includes/Rest/Handler.php:184
/workspace/src/tests/phpunit/unit/includes/Rest/Handler/HandlerTestTrait.php:91
/workspace/src/tests/phpunit/unit/includes/Rest/Handler/HandlerTestTrait.php:143
/workspace/src/tests/phpunit/unit/includes/Rest/Handler/HandlerTestTrait.php:185
/workspace/src/extensions/CampaignEvents/tests/phpunit/unit/Rest/ListEventsByOrganizerHandlerTest.php:62
/workspace/src/tests/phpunit/MediaWikiUnitTestCase.php:111
BPirkle subscribed.

Just ran into this while working on REST endpoints for the Reading Lists extension. Tagging the MediaWiki Interfaces team for visibility and triage.

Wikimedia\Services\ServiceContainer::get('TitleFactory') was not expected to be called.

If the test is a subclass of MediaWikiIntegrationTestCase, this shoujld work, since then we have a real Title factory. That'S probably the easiest fix, but not nice.

Injecting a validator callback would be better, but the methods in HandlerTestTrait are already really long and complicated. They should probably take associative arrays (or we just depend on named parameters becoming available in php 8).

I think for "unit" tests, it would be good if we could inject a ParamValidator. Then we could check for the handling of validatio nerrors, without doing real validation.

Triaging this as Low priority, mostly because it has been around quite some time and it seems people are working around this for the moment. Feel to raise the priority if this is a blocker for anything you're doing.

Fixed by r1023884?

Close. You'd still need to do something like this:

protected function setUp(): void {
		parent::setUp();
		$this->setService(
			'TitleFactory',
			$this->makeMockTitleFactory()
		);
	}
daniel claimed this task.

I think the above is good enough. Handler tests based on MediaWikiUnitTestCase will have to set up the services necessary for checking the types of parameters they need.

This could be more obvious and convenient, but I don't know how. We can have a new ticket when there's a proposal for that.