Jeroen De Dauw has uploaded a new change for review. https://gerrit.wikimedia.org/r/67526
Change subject: Got rid of MWException in Claims and improved its tests ...................................................................... Got rid of MWException in Claims and improved its tests Change-Id: Ie53aa8a666e3bc6cf5bdfa016af60cf6f479034c --- M DataModel/DataModel/Claim/Claims.php M DataModel/tests/phpunit/Claim/ClaimsTest.php 2 files changed, 79 insertions(+), 54 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase refs/changes/26/67526/1 diff --git a/DataModel/DataModel/Claim/Claims.php b/DataModel/DataModel/Claim/Claims.php index 0842a6d..660c8ea 100644 --- a/DataModel/DataModel/Claim/Claims.php +++ b/DataModel/DataModel/Claim/Claims.php @@ -3,7 +3,11 @@ namespace Wikibase; use Diff\Differ; -use MWException; +use Diff\DiffOpAdd; +use Diff\DiffOpChange; +use Diff\DiffOpRemove; +use Diff\MapDiffer; +use InvalidArgumentException; /** * Implementation of the Claims interface. @@ -168,12 +172,12 @@ * * @param int $propertyId * - * @throws \MWException if $propertyId isn't valid + * @throws InvalidArgumentException * @return Claims */ public function getClaimsForProperty( $propertyId ) { if ( !is_int( $propertyId ) ) { - throw new MWException( "ID must be an int." ); + throw new InvalidArgumentException( '$propertyId must be an integer' ); } $claimsByProp = new ByPropertyIdArray( $this ); @@ -251,14 +255,14 @@ * @param Differ|null $differ * * @return \Diff\Diff - * @throws MWException + * @throws InvalidArgumentException */ public function getDiff( Claims $claims, Differ $differ = null ) { assert( $this->indicesAreUpToDate() ); assert( $claims->indicesAreUpToDate() ); if ( $differ === null ) { - $differ = new \Diff\MapDiffer(); + $differ = new MapDiffer(); } $sourceHashes = array(); @@ -278,7 +282,7 @@ $diff = new \Diff\Diff( array(), true ); foreach ( $differ->doDiff( $sourceHashes, $targetHashes ) as $diffOp ) { - if ( $diffOp instanceof \Diff\DiffOpChange ) { + if ( $diffOp instanceof DiffOpChange ) { $oldClaim = $this->getByElementHash( $diffOp->getOldValue() ); $newClaim = $claims->getByElementHash( $diffOp->getNewValue() ); @@ -286,20 +290,20 @@ assert( $newClaim instanceof Claim ); assert( $oldClaim->getGuid() === $newClaim->getGuid() ); - $diff[$oldClaim->getGuid()] = new \Diff\DiffOpChange( $oldClaim, $newClaim ); + $diff[$oldClaim->getGuid()] = new DiffOpChange( $oldClaim, $newClaim ); } - elseif ( $diffOp instanceof \Diff\DiffOpAdd ) { + elseif ( $diffOp instanceof DiffOpAdd ) { $claim = $claims->getByElementHash( $diffOp->getNewValue() ); assert( $claim instanceof Claim ); - $diff[$claim->getGuid()] = new \Diff\DiffOpAdd( $claim ); + $diff[$claim->getGuid()] = new DiffOpAdd( $claim ); } - elseif ( $diffOp instanceof \Diff\DiffOpRemove ) { + elseif ( $diffOp instanceof DiffOpRemove ) { $claim = $this->getByElementHash( $diffOp->getOldValue() ); assert( $claim instanceof Claim ); - $diff[$claim->getGuid()] = new \Diff\DiffOpRemove( $claim ); + $diff[$claim->getGuid()] = new DiffOpRemove( $claim ); } else { - throw new MWException( 'Invalid DiffOp type cannot be handled' ); + throw new InvalidArgumentException( 'Invalid DiffOp type cannot be handled' ); } } diff --git a/DataModel/tests/phpunit/Claim/ClaimsTest.php b/DataModel/tests/phpunit/Claim/ClaimsTest.php index 8a895b7..35e7961 100644 --- a/DataModel/tests/phpunit/Claim/ClaimsTest.php +++ b/DataModel/tests/phpunit/Claim/ClaimsTest.php @@ -2,11 +2,25 @@ namespace Wikibase\Test; +use DataValues\StringValue; +use Diff\Diff; +use Diff\DiffOpAdd; +use Diff\DiffOpChange; +use Diff\DiffOpRemove; use Wikibase\Claim; use Wikibase\Claims; +use Wikibase\EntityId; +use Wikibase\Property; +use Wikibase\PropertyNoValueSnak; +use Wikibase\PropertySomeValueSnak; +use Wikibase\PropertyValueSnak; +use Wikibase\Reference; +use Wikibase\ReferenceList; +use Wikibase\SnakList; +use Wikibase\Statement; /** - * Tests for the Wikibase\Claims class. + * @covers Wikibase\Claims * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -57,18 +71,18 @@ public function getElementInstances() { $instances = array(); - $instances[] = new \Wikibase\Claim( - new \Wikibase\PropertyNoValueSnak( - new \Wikibase\EntityId( \Wikibase\Property::ENTITY_TYPE, 23 ) ) ); + $instances[] = new Claim( + new PropertyNoValueSnak( + new EntityId( Property::ENTITY_TYPE, 23 ) ) ); - $instances[] = new \Wikibase\Claim( - new \Wikibase\PropertySomeValueSnak( - new \Wikibase\EntityId( \Wikibase\Property::ENTITY_TYPE, 42 ) ) ); + $instances[] = new Claim( + new PropertySomeValueSnak( + new EntityId( Property::ENTITY_TYPE, 42 ) ) ); - $instances[] = new \Wikibase\Claim( - new \Wikibase\PropertyValueSnak( - new \Wikibase\EntityId( \Wikibase\Property::ENTITY_TYPE, 42 ), - new \DataValues\StringValue( "foo" ) ) ); + $instances[] = new Claim( + new PropertyValueSnak( + new EntityId( Property::ENTITY_TYPE, 42 ), + new StringValue( "foo" ) ) ); return $instances; } @@ -174,8 +188,8 @@ } public function testDuplicateClaims() { - $firstClaim = new Claim( new \Wikibase\PropertyNoValueSnak( 42 ) ); - $secondClaim = new Claim( new \Wikibase\PropertyNoValueSnak( 42 ) ); + $firstClaim = new Claim( new PropertyNoValueSnak( 42 ) ); + $secondClaim = new Claim( new PropertyNoValueSnak( 42 ) ); $list = new Claims(); $this->assertTrue( $list->addElement( $firstClaim ), 'Adding the first element should work' ); @@ -183,7 +197,7 @@ $this->assertEquals( 2, count( $list->getArrayCopy() ), 'Adding two duplicates to an empty list should result in a count of two' ); - $this->assertTrue( $list->addElement( new Claim( new \Wikibase\PropertySomeValueSnak( 1 ) ) ) ); + $this->assertTrue( $list->addElement( new Claim( new PropertySomeValueSnak( 1 ) ) ) ); $list->removeDuplicates(); @@ -193,18 +207,18 @@ public function getDiffProvider() { $argLists = array(); - $claim0 = new Claim( new \Wikibase\PropertyNoValueSnak( 42 ) ); - $claim1 = new Claim( new \Wikibase\PropertySomeValueSnak( 42 ) ); - $claim2 = new Claim( new \Wikibase\PropertyValueSnak( 42, new \DataValues\StringValue( 'ohi' ) ) ); - $claim3 = new Claim( new \Wikibase\PropertyNoValueSnak( 1 ) ); - $claim4 = new Claim( new \Wikibase\PropertyNoValueSnak( 2 ) ); + $claim0 = new Claim( new PropertyNoValueSnak( 42 ) ); + $claim1 = new Claim( new PropertySomeValueSnak( 42 ) ); + $claim2 = new Claim( new PropertyValueSnak( 42, new StringValue( 'ohi' ) ) ); + $claim3 = new Claim( new PropertyNoValueSnak( 1 ) ); + $claim4 = new Claim( new PropertyNoValueSnak( 2 ) ); - $statement0 = new \Wikibase\Statement( new \Wikibase\PropertyNoValueSnak( 5 ) ); - $statement0->setRank( \Wikibase\Statement::RANK_PREFERRED ); + $statement0 = new Statement( new PropertyNoValueSnak( 5 ) ); + $statement0->setRank( Statement::RANK_PREFERRED ); - $statement1 = new \Wikibase\Statement( new \Wikibase\PropertyNoValueSnak( 5 ) ); - $statement1->setReferences( new \Wikibase\ReferenceList( array( new \Wikibase\Reference( - new \Wikibase\SnakList( array( new \Wikibase\PropertyValueSnak( 10, new \DataValues\StringValue( 'spam' ) ) ) ) + $statement1 = new Statement( new PropertyNoValueSnak( 5 ) ); + $statement1->setReferences( new ReferenceList( array( new Reference( + new SnakList( array( new PropertyValueSnak( 10, new StringValue( 'spam' ) ) ) ) ) ) ) ); $claim0->setGuid( 'claim0' ); @@ -214,68 +228,68 @@ $statement0->setGuid( 'statement0' ); $claim2v2 = unserialize( serialize( $claim2 ) ); - $claim2v2->setMainSnak( new \Wikibase\PropertyValueSnak( 42, new \DataValues\StringValue( 'omnomnom' ) ) ); + $claim2v2->setMainSnak( new PropertyValueSnak( 42, new StringValue( 'omnomnom' ) ) ); $source = new Claims(); $target = new Claims(); - $expected = new \Diff\Diff( array(), true ); + $expected = new Diff( array(), true ); $argLists[] = array( $source, $target, $expected, 'Two empty lists should result in an empty diff' ); $source = new Claims(); $target = new Claims( array( $claim0 ) ); - $expected = new \Diff\Diff( array( $claim0->getGuid() => new \Diff\DiffOpAdd( $claim0 ) ), true ); + $expected = new Diff( array( $claim0->getGuid() => new DiffOpAdd( $claim0 ) ), true ); $argLists[] = array( $source, $target, $expected, 'List with no entries to list with one should result in one add op' ); $source = new Claims( array( $claim0 ) ); $target = new Claims(); - $expected = new \Diff\Diff( array( $claim0->getGuid() => new \Diff\DiffOpRemove( $claim0 ) ), true ); + $expected = new Diff( array( $claim0->getGuid() => new DiffOpRemove( $claim0 ) ), true ); $argLists[] = array( $source, $target, $expected, 'List with one entry to an empty list should result in one remove op' ); $source = new Claims( array( $claim0, $claim3, $claim2 ) ); $target = new Claims( array( $claim0, $claim2, $claim3 ) ); - $expected = new \Diff\Diff( array(), true ); + $expected = new Diff( array(), true ); $argLists[] = array( $source, $target, $expected, 'Two identical lists should result in an empty diff' ); $source = new Claims( array( $claim0 ) ); $target = new Claims( array( $claim1 ) ); - $expected = new \Diff\Diff( array( - $claim1->getGuid() => new \Diff\DiffOpAdd( $claim1 ), - $claim0->getGuid() => new \Diff\DiffOpRemove( $claim0 ) + $expected = new Diff( array( + $claim1->getGuid() => new DiffOpAdd( $claim1 ), + $claim0->getGuid() => new DiffOpRemove( $claim0 ) ), true ); $argLists[] = array( $source, $target, $expected, 'Two lists with each a single different entry should result into one add and one remove op' ); $source = new Claims( array( $claim2, $claim3, $claim0, $claim4 ) ); $target = new Claims( array( $claim2, $claim1, $claim3, $claim4 ) ); - $expected = new \Diff\Diff( array( - $claim1->getGuid() => new \Diff\DiffOpAdd( $claim1 ), - $claim0->getGuid() => new \Diff\DiffOpRemove( $claim0 ) + $expected = new Diff( array( + $claim1->getGuid() => new DiffOpAdd( $claim1 ), + $claim0->getGuid() => new DiffOpRemove( $claim0 ) ), true ); $argLists[] = array( $source, $target, $expected, 'Two lists with identical items except for one change should result in one add and one remove op' ); $source = new Claims( array( $claim0, $claim0, $claim3, $claim2, $claim2, $claim2, $statement0 ) ); $target = new Claims( array( $claim0, $claim0, $claim2, $claim3, $claim2, $claim2, $statement0 ) ); - $expected = new \Diff\Diff( array(), true ); + $expected = new Diff( array(), true ); $argLists[] = array( $source, $target, $expected, 'Two identical lists with duplicate items should result in an empty diff' ); $source = new Claims( array( $statement0, $statement1, $claim0 ) ); $target = new Claims( array( $claim1, $claim1, $claim0, $statement1 ) ); - $expected = new \Diff\Diff( array( - $claim1->getGuid() => new \Diff\DiffOpAdd( $claim1 ), - $statement0->getGuid() => new \Diff\DiffOpRemove( $statement0 ), + $expected = new Diff( array( + $claim1->getGuid() => new DiffOpAdd( $claim1 ), + $statement0->getGuid() => new DiffOpRemove( $statement0 ), ), true ); $argLists[] = array( $source, $target, $expected, 'Two lists with duplicate items and a different entry should result into one add and one remove op' ); $source = new Claims( array( $claim0, $claim3, $claim2 ) ); $target = new Claims( array( $claim0, $claim2v2, $claim3 ) ); - $expected = new \Diff\Diff( array( $claim2->getGuid() => new \Diff\DiffOpChange( $claim2, $claim2v2 ) ), true ); + $expected = new Diff( array( $claim2->getGuid() => new DiffOpChange( $claim2, $claim2v2 ) ), true ); $argLists[] = array( $source, $target, $expected, 'Changing the value of a claim should result in a change op' ); return $argLists; @@ -286,14 +300,21 @@ * * @param \Wikibase\Claims $source * @param \Wikibase\Claims $target - * @param \Diff\Diff $expected + * @param Diff $expected * @param string $message */ - public function testGetDiff( Claims $source, Claims $target, \Diff\Diff $expected, $message ) { + public function testGetDiff( Claims $source, Claims $target, Diff $expected, $message ) { $actual = $source->getDiff( $target ); // Note: this makes order of inner arrays relevant, and this order is not guaranteed by the interface $this->assertEquals( $expected->getOperations(), $actual->getOperations(), $message ); } + public function testCallingGetClaimsForPropertyWithInvalidArgumentCausesException() { + $claims = new Claims(); + + $this->setExpectedException( 'InvalidArgumentException' ); + $claims->getClaimsForProperty( 'foo bar' ); + } + } -- To view, visit https://gerrit.wikimedia.org/r/67526 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ie53aa8a666e3bc6cf5bdfa016af60cf6f479034c Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: master Gerrit-Owner: Jeroen De Dauw <jeroended...@gmail.com> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits