Page MenuHomePhabricator

PHPUnit: Cover some model/MobilePage class methods
Closed, ResolvedPublic

Description

The class in model/MobilePage.php file has no code coverage. Tests should be written to fix this.

Acceptance criteria

  • Write unit tests to cover some methods in /models/MobilePage.php - not all methods covered
  • Code coverage report has gone up - from 0% to 21.31%
  • Covers other methods for completeness to 100%
  • Code coverage report has gone up

Event Timeline

D3r1ck01 triaged this task as Normal priority.Nov 14 2018, 11:50 AM
D3r1ck01 created this task.
D3r1ck01 removed D3r1ck01 as the assignee of this task.Nov 14 2018, 11:53 AM

Will grab this soon! There is a pending patch that will be continued, see here: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/472427.

D3r1ck01 updated the task description. (Show Details)Nov 14 2018, 11:58 AM

Please keep this in the backlog. I'll move your tasks into our sprint board when they need review so they don't confuse our product owner :)

Change 472427 had a related patch set uploaded (by Jdlrobson; owner: Alangi Derick):
[mediawiki/extensions/MobileFrontend@master] Add MobilePageTest to test MobilePage::class

https://gerrit.wikimedia.org/r/472427

Okay now understanding what happened here :)

Not this one yet @Jdlrobson, I'm working on this: T209477 instead now :)

D3r1ck01 moved this task from Backlog to Doing [WIP] on the User-D3r1ck01 board.Nov 14 2018, 9:27 PM

Change 472427 had a related patch set uploaded (by D3r1ck01; owner: Alangi Derick):
[mediawiki/extensions/MobileFrontend@master] Add MobilePageTest to test MobilePage::class

https://gerrit.wikimedia.org/r/472427

D3r1ck01 removed D3r1ck01 as the assignee of this task.Nov 16 2018, 4:53 PM
D3r1ck01 removed a project: Patch-For-Review.
D3r1ck01 moved this task from Doing [WIP] to Backlog on the User-D3r1ck01 board.

There is a work in progress patch that needs some discussions on the way forward :)

here's a patch for what we talked about over hangouts:

Thanks for the patch @Jdlrobson. I'm not sure but I'm having a feeling about the mocking related to;

->method( 'transform' )
->will( $this->returnSelf() )

That piece of code is related to the mock freaking out at some point :)

@Jdlrobson, I was able to reproduce this issue locally so it's all around the PageImages extension dependency. I've commented on the patchset so that PageImages dependency is currently blocking this and I think we should create a task and refactor that, before moving on with this!

D3r1ck01 moved this task from Backlog to Doing [WIP] on the User-D3r1ck01 board.Nov 17 2018, 5:21 PM
D3r1ck01 added a project: Patch-For-Review.

Blocker (T209768: Fix $this->file hard dependency on PageImages extension) has been solved so we can move on with this now :)

D3r1ck01 claimed this task.Nov 17 2018, 5:23 PM

@Jdlrobson, while working on MobilePageTest, I realized something in the MobilePage class. Is this code use?

/**
* Set rev_timestamp of latest edit to this page
* @param string $timestamp Timestamp (MW format)
*/
public function setLatestTimestamp( $timestamp ) {
	$this->revisionTimestamp = $timestamp;
}

It's not called anywhere in the code base, usage is 0 so I'm wondering the use of the code there.

D3r1ck01 updated the task description. (Show Details)Nov 17 2018, 7:56 PM

Will halt work on this file for now but per the goal, will revisit it again in the nearest future :)

D3r1ck01 renamed this task from PHPUnit: Cover all model/MobilePage class methods to PHPUnit: Cover some model/MobilePage class methods.Nov 17 2018, 8:00 PM
D3r1ck01 updated the task description. (Show Details)
D3r1ck01 moved this task from Doing [WIP] to Completed (To be Resolved) on the User-D3r1ck01 board.

Hello @pmiazga, thanks for your detailed review ;}, but T209768 currently blocks the parent, could you review it first please? Before we I dwell on the MobilePage class proper? :)

Change 474958 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] SpecialMobileEditWatchlist shouldn't query PageImages extension twice

https://gerrit.wikimedia.org/r/474958

D3r1ck01 updated the task description. (Show Details)Nov 20 2018, 6:35 PM
D3r1ck01 moved this task from Doing [WIP] to Completed (To be Resolved) on the User-D3r1ck01 board.

Change 474958 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] SpecialMobileEditWatchlist shouldn't query PageImages extension twice

https://gerrit.wikimedia.org/r/474958

Change 472427 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add PHPUnit to tests for methods in MobilePage::class

https://gerrit.wikimedia.org/r/472427

D3r1ck01 updated the task description. (Show Details)

Change 475041 had a related patch set uploaded (by D3r1ck01; owner: Alangi Derick):
[mediawiki/extensions/MobileFrontend@master] Add PHPUnit tests for more paths in MobilePage class

https://gerrit.wikimedia.org/r/475041

@Jdlrobson & @pmiazga, following a dependency approach to test getLatestEdit() method as I was trying to avoid DB query/queries has failed me as I can't inject a mock into the MobilePage class. Here is the mock I came up with but no way to inject :(

/**
  * Mock a MobilePage class to with revisions for testing getLatestEdit() method.
  * Note that DB queries should be avoided (very expensive) when doing testing.
  */
private function mockMobilePageWithRevisionFactory() {
	// Firstly, mock the Revision class
        $mockRevision = $this->getMockBuilder( 'Revision' )
		->disableOriginalConstructor()
		->setMethods(
			[ 'getTimestamp', 'getUser', 'getName', 'getOptions', 'getRevision' ]
		)
		->getMock();

	$mockRevision->expects( $this->once() )
		->method( 'getTimestamp' )
		->willReturn( '20181028200709' );

	$mockRevision->expects( $this->once() )
		->method( 'getUser' )
		->willReturn( 'TestUser' );

	$mockRevision->expects( $this->once() )
		->method( 'getName' )
		->willReturn( 'TestUserName' );

	$mockRevision->expects( $this->once() )
		->method( 'getOptions' )
		->willReturn( 'Male' );

	// Now, mock the MobilePage class
	$mockMobilePage = $this->getMockBuilder( MobilePage::class )
		->disableOriginalConstructor()
		->setMethods( [ 'getLatestEdit', 'getRevision' ] )
		->getMock();

	$mockMobilePage->expects( $this->once() )
		->method( 'getRevision' )
		->willReturn( $mockRevision );

	$mockMobilePage->expects( $this->any() )
		->method( 'getLatestEdit' )
		->willReturn( `No way to inject???` );

	return $mockMobilePage;
}

So I'll have to follow the route for querying db (just 1 query here) but which in ideal cases should be avoided for unit tests :(

D3r1ck01 added a comment.EditedNov 21 2018, 9:44 AM

So following the route that will query the db, the following assertions should do;

$this->assertInternalType( 'array', $actual );
$this->assertArrayHasKey( 'timestamp', $actual );
$this->assertArrayHasKey( 'name', $actual );
$this->assertArrayHasKey( 'gender', $actual );

This route will hold test coverage from not reaching 100% as it seems unit tests do not run DB queries :(

D3r1ck01 added a comment.EditedNov 22 2018, 4:50 PM

Really funny sometimes how tools work ;), so the actual issue for us not reaching 100% coverage was due to an early return in a function that makes executing after not done. See image below for the cause;

There should be a scenario that handles the case of when the second if condition (if ( $thumb && $thumb->getUrl() ) {) fails, maybe just return ''?

D3r1ck01 updated the task description. (Show Details)Nov 22 2018, 8:03 PM
D3r1ck01 updated the task description. (Show Details)Nov 23 2018, 3:55 PM
D3r1ck01 awarded a token.

Change 475041 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add PHPUnit tests for more paths in MobilePage::class

https://gerrit.wikimedia.org/r/475041

Jdlrobson closed this task as Resolved.Nov 23 2018, 5:34 PM

The task is about covering "some" but you covered it all!
https://doc.wikimedia.org/cover-extensions/MobileFrontend/models/index.html
Good job and great to see 100% coverage in an entire folder! w00t!

D3r1ck01 added a comment.EditedNov 23 2018, 6:37 PM

Good job and great to see 100% coverage in an entire folder! w00t!

Thanks a lot @Jdlrobson, please extend my thanks to @pmiazga. 100% is the plan and we need to keep that streak :)