Thiemo Mättig (WMDE) has uploaded a new change for review.
https://gerrit.wikimedia.org/r/275551
Change subject: Fix serialization order, streamline and strengthen tests
......................................................................
Fix serialization order, streamline and strengthen tests
This fixes most issues I had with the 3 previous patches.
* Fix serialization order (should be type, id, …).
* Favor assertSame when possible.
* Use test providers.
Change-Id: I6f6e75ebb97b3b78de09a00ff61bae93dcfc728b
---
M src/DataModel/MediaInfo.php
M src/DataModel/Serialization/MediaInfoSerializer.php
M tests/phpunit/integration/WikibaseMediaInfoHooksTest.php
M tests/phpunit/unit/DatatModel/MediaInfoIdTest.php
M tests/phpunit/unit/DatatModel/MediaInfoTest.php
M tests/phpunit/unit/DatatModel/Serialization/MediaInfoSerializerTest.php
M tests/phpunit/unit/WikibaseMediaInfoHooksTest.php
7 files changed, 146 insertions(+), 119 deletions(-)
git pull
ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/WikibaseMediaInfo
refs/changes/51/275551/1
diff --git a/src/DataModel/MediaInfo.php b/src/DataModel/MediaInfo.php
index ea8b9f8..83c7e48 100644
--- a/src/DataModel/MediaInfo.php
+++ b/src/DataModel/MediaInfo.php
@@ -78,6 +78,8 @@
/**
* @param MediaInfoId $id
+ *
+ * @throws InvalidArgumentException
*/
public function setId( $id ) {
if ( !( $id instanceof MediaInfoId ) ) {
@@ -148,8 +150,9 @@
* @see http://php.net/manual/en/language.oop5.cloning.php
*/
public function __clone() {
- $this->labels = unserialize( serialize( $this->labels ) );
- $this->descriptions = unserialize( serialize(
$this->descriptions ) );
+ $this->labels = clone $this->labels;
+ $this->descriptions = clone $this->descriptions;
+ // TODO: DataModel 5.1 is needed to be able to use clone for
the statements.
$this->statements = unserialize( serialize( $this->statements )
);
}
diff --git a/src/DataModel/Serialization/MediaInfoSerializer.php
b/src/DataModel/Serialization/MediaInfoSerializer.php
index 995ccbe..f77f892 100644
--- a/src/DataModel/Serialization/MediaInfoSerializer.php
+++ b/src/DataModel/Serialization/MediaInfoSerializer.php
@@ -67,12 +67,7 @@
}
private function getSerialized( MediaInfo $mediaInfo ) {
- $serialization = [
- 'type' => $mediaInfo->getType(),
- 'labels' => $this->termListSerializer->serialize(
$mediaInfo->getLabels() ),
- 'descriptions' => $this->termListSerializer->serialize(
$mediaInfo->getDescriptions() ),
- 'statements' =>
$this->statementListSerializer->serialize( $mediaInfo->getStatements() )
- ];
+ $serialization = [ 'type' => $mediaInfo->getType() ];
$id = $mediaInfo->getId();
@@ -80,6 +75,14 @@
$serialization['id'] = $id->getSerialization();
}
+ $serialization['labels'] =
$this->termListSerializer->serialize( $mediaInfo->getLabels() );
+ $serialization['descriptions'] =
$this->termListSerializer->serialize(
+ $mediaInfo->getDescriptions()
+ );
+ $serialization['statements'] =
$this->statementListSerializer->serialize(
+ $mediaInfo->getStatements()
+ );
+
return $serialization;
}
diff --git a/tests/phpunit/integration/WikibaseMediaInfoHooksTest.php
b/tests/phpunit/integration/WikibaseMediaInfoHooksTest.php
index 55a95b9..8bfce7c 100644
--- a/tests/phpunit/integration/WikibaseMediaInfoHooksTest.php
+++ b/tests/phpunit/integration/WikibaseMediaInfoHooksTest.php
@@ -32,7 +32,7 @@
Hooks::run( 'WikibaseRepoEntityTypes', [
&$entityTypeDefinitions ] );
$this->assertArrayHasKey( 'item', $entityTypeDefinitions );
- $this->assertEquals( [ 'foo', 'bar' ],
$entityTypeDefinitions['item'] );
+ $this->assertSame( [ 'foo', 'bar' ],
$entityTypeDefinitions['item'] );
$this->assertArrayHasKey( 'mediainfo', $entityTypeDefinitions );
}
@@ -45,7 +45,7 @@
Hooks::run( 'WikibaseClientEntityTypes', [
&$entityTypeDefinitions ] );
$this->assertArrayHasKey( 'item', $entityTypeDefinitions );
- $this->assertEquals( [ 'foo', 'bar' ],
$entityTypeDefinitions['item'] );
+ $this->assertSame( [ 'foo', 'bar' ],
$entityTypeDefinitions['item'] );
$this->assertArrayHasKey( 'mediainfo', $entityTypeDefinitions );
}
diff --git a/tests/phpunit/unit/DatatModel/MediaInfoIdTest.php
b/tests/phpunit/unit/DatatModel/MediaInfoIdTest.php
index 1cf9aac..83ad1e3 100644
--- a/tests/phpunit/unit/DatatModel/MediaInfoIdTest.php
+++ b/tests/phpunit/unit/DatatModel/MediaInfoIdTest.php
@@ -29,7 +29,7 @@
public function testValidIds( $serialization ) {
$id = new MediaInfoId( $serialization );
- $this->assertEquals( strtoupper( $serialization ),
$id->getSerialization() );
+ $this->assertSame( strtoupper( $serialization ),
$id->getSerialization() );
}
public function provideInvalidIds() {
@@ -56,30 +56,29 @@
/**
* @dataProvider provideInvalidIds
- * @expectedException InvalidArgumentException
*/
public function testInvalidIds( $serialization ) {
+ $this->setExpectedException( InvalidArgumentException::class );
new MediaInfoId( $serialization );
}
public function testGetEntityType() {
$id = new MediaInfoId( 'M1' );
- $this->assertEquals( 'mediainfo', $id->getEntityType() );
+ $this->assertSame( 'mediainfo', $id->getEntityType() );
}
public function testSerialize() {
$id = new MediaInfoId( 'M1' );
- $this->assertEquals( 'M1', $id->serialize() );
+ $this->assertSame( 'M1', serialize( $id ) );
}
/**
* @dataProvider serializationProvider
*/
public function testUnserialize( $serialization ) {
- $id = new MediaInfoId( 'M1' );
- $id->unserialize( $serialization );
+ $id = unserialize( $serialization );
$this->assertSame( $serialization, $id->getSerialization() );
}
diff --git a/tests/phpunit/unit/DatatModel/MediaInfoTest.php
b/tests/phpunit/unit/DatatModel/MediaInfoTest.php
index 0fb99cd..56087a5 100644
--- a/tests/phpunit/unit/DatatModel/MediaInfoTest.php
+++ b/tests/phpunit/unit/DatatModel/MediaInfoTest.php
@@ -7,6 +7,7 @@
use Wikibase\DataModel\Entity\Item;
use Wikibase\DataModel\Entity\ItemId;
use Wikibase\DataModel\Snak\PropertyNoValueSnak;
+use Wikibase\DataModel\Statement\Statement;
use Wikibase\DataModel\Statement\StatementList;
use Wikibase\DataModel\Term\TermList;
use Wikibase\MediaInfo\DataModel\MediaInfo;
@@ -46,7 +47,7 @@
public function testGetEntityType() {
$mediaInfo = new MediaInfo();
- $this->assertEquals( 'mediainfo', $mediaInfo->getType() );
+ $this->assertSame( 'mediainfo', $mediaInfo->getType() );
}
public function testSetNewId() {
@@ -77,10 +78,11 @@
/**
* @dataProvider provideInvalidIds
- * @expectedException InvalidArgumentException
*/
public function testSetInvalidId( $id ) {
$mediaInfo = new MediaInfo();
+
+ $this->setExpectedException( InvalidArgumentException::class );
$mediaInfo->setId( $id );
}
@@ -117,106 +119,109 @@
$this->assertFalse( $mediaInfo->isEmpty() );
}
- public function testEqualsEmpty() {
- $mediaInfo = new MediaInfo();
+ public function provideEqualEntities() {
+ $empty = new MediaInfo();
- $this->assertTrue( $mediaInfo->equals( new MediaInfo() ) );
+ $withLabel = new MediaInfo();
+ $withLabel->getLabels()->setTextForLanguage( 'en', 'Foo' );
+
+ $withDescription = new MediaInfo();
+ $withDescription->getDescriptions()->setTextForLanguage( 'en',
'Foo' );
+
+ $withStatement = new MediaInfo();
+ $withStatement->getStatements()->addNewStatement( new
PropertyNoValueSnak( 42 ) );
+
+ return [
+ 'empty' => [
+ $empty,
+ new MediaInfo()
+ ],
+ 'same id' => [
+ new MediaInfo( new MediaInfoId( 'M1' ) ),
+ new MediaInfo( new MediaInfoId( 'M1' ) )
+ ],
+ 'different id' => [
+ new MediaInfo( new MediaInfoId( 'M1' ) ),
+ new MediaInfo( new MediaInfoId( 'M2' ) )
+ ],
+ 'no id' => [
+ new MediaInfo( new MediaInfoId( 'M1' ) ),
+ $empty
+ ],
+ 'same object' => [
+ $empty,
+ $empty
+ ],
+ 'same labels' => [
+ $withLabel,
+ clone $withLabel
+ ],
+ 'same descriptions' => [
+ $withDescription,
+ clone $withDescription
+ ],
+ 'same statements' => [
+ $withStatement,
+ clone $withStatement
+ ],
+ ];
}
- public function testEqualsSameId() {
- $mediaInfo = new MediaInfo( new MediaInfoId( 'M1' ) );
-
- $this->assertTrue( $mediaInfo->equals( new MediaInfo( new
MediaInfoId( 'M1' ) ) ) );
+ /**
+ * @dataProvider provideEqualEntities
+ */
+ public function testEquals( MediaInfo $a, MediaInfo $b ) {
+ $this->assertTrue( $a->equals( $b ) );
}
- public function testEqualsDifferentId() {
- $mediaInfo = new MediaInfo( new MediaInfoId( 'M1' ) );
+ public function provideNotEqualEntities() {
+ $withLabel1 = new MediaInfo();
+ $withLabel1->getLabels()->setTextForLanguage( 'en', 'Foo' );
- $this->assertTrue( $mediaInfo->equals( new MediaInfo( new
MediaInfoId( 'M2' ) ) ) );
+ $withLabel2 = new MediaInfo();
+ $withLabel2->getLabels()->setTextForLanguage( 'en', 'Bar' );
+
+ $withDescription1 = new MediaInfo();
+ $withDescription1->getDescriptions()->setTextForLanguage( 'en',
'Foo' );
+
+ $withDescription2 = new MediaInfo();
+ $withDescription2->getDescriptions()->setTextForLanguage( 'en',
'Bar' );
+
+ $withStatement1 = new MediaInfo();
+ $withStatement1->getStatements()->addNewStatement( new
PropertyNoValueSnak( 42 ) );
+
+ $withStatement2 = new MediaInfo();
+ $withStatement2->getStatements()->addNewStatement( new
PropertyNoValueSnak( 24 ) );
+
+ return [
+ 'null' => [
+ new MediaInfo(),
+ null
+ ],
+ 'item' => [
+ new MediaInfo(),
+ new Item()
+ ],
+ 'different labels' => [
+ $withLabel1,
+ $withLabel2
+ ],
+ 'different descriptions' => [
+ $withDescription1,
+ $withDescription2
+ ],
+ 'different statements' => [
+ $withStatement1,
+ $withStatement2
+ ],
+ ];
}
- public function testEqualsNoId() {
- $mediaInfo = new MediaInfo( new MediaInfoId( 'M1' ) );
-
- $this->assertTrue( $mediaInfo->equals( new MediaInfo() ) );
- }
-
- public function testEqualsSameObject() {
- $mediaInfo = new MediaInfo();
-
- $this->assertTrue( $mediaInfo->equals( $mediaInfo ) );
- }
-
- public function testEqualsSameLabels() {
- $mediaInfo1 = new MediaInfo();
- $mediaInfo2 = new MediaInfo();
-
- $mediaInfo1->getLabels()->setTextForLanguage( 'en', 'Foo' );
- $mediaInfo2->getLabels()->setTextForLanguage( 'en', 'Foo' );
-
- $this->assertTrue( $mediaInfo1->equals( $mediaInfo2 ) );
- }
-
- public function testEqualsSameDescriptions() {
- $mediaInfo1 = new MediaInfo();
- $mediaInfo2 = new MediaInfo();
-
- $mediaInfo1->getDescriptions()->setTextForLanguage( 'en', 'Foo'
);
- $mediaInfo2->getDescriptions()->setTextForLanguage( 'en', 'Foo'
);
-
- $this->assertTrue( $mediaInfo1->equals( $mediaInfo2 ) );
- }
-
- public function testEqualsSameStatements() {
- $mediaInfo1 = new MediaInfo();
- $mediaInfo2 = new MediaInfo();
-
- $mediaInfo1->getStatements()->addNewStatement( new
PropertyNoValueSnak( 42 ) );
- $mediaInfo2->getStatements()->addNewStatement( new
PropertyNoValueSnak( 42 ) );
-
- $this->assertTrue( $mediaInfo1->equals( $mediaInfo2 ) );
- }
-
- public function testNotEqualsNull() {
- $mediaInfo = new MediaInfo();
-
- $this->assertFalse( $mediaInfo->equals( null ) );
- }
-
- public function testNotEqualsItem() {
- $mediaInfo = new MediaInfo();
-
- $this->assertFalse( $mediaInfo->equals( new Item() ) );
- }
-
- public function testNotEqualsDifferentLabels() {
- $mediaInfo1 = new MediaInfo();
- $mediaInfo2 = new MediaInfo();
-
- $mediaInfo1->getLabels()->setTextForLanguage( 'en', 'Foo' );
- $mediaInfo2->getLabels()->setTextForLanguage( 'en', 'Bar' );
-
- $this->assertFalse( $mediaInfo1->equals( $mediaInfo2 ) );
- }
-
- public function testNotEqualsDifferentDescriptions() {
- $mediaInfo1 = new MediaInfo();
- $mediaInfo2 = new MediaInfo();
-
- $mediaInfo1->getDescriptions()->setTextForLanguage( 'en', 'Foo'
);
- $mediaInfo2->getDescriptions()->setTextForLanguage( 'en', 'Bar'
);
-
- $this->assertFalse( $mediaInfo1->equals( $mediaInfo2 ) );
- }
-
- public function testNotEqualsDifferentStatements() {
- $mediaInfo1 = new MediaInfo();
- $mediaInfo2 = new MediaInfo();
-
- $mediaInfo1->getStatements()->addNewStatement( new
PropertyNoValueSnak( 42 ) );
- $mediaInfo2->getStatements()->addNewStatement( new
PropertyNoValueSnak( 24 ) );
-
- $this->assertFalse( $mediaInfo1->equals( $mediaInfo2 ) );
+ /**
+ * @dataProvider provideNotEqualEntities
+ */
+ public function testNotEquals( MediaInfo $a, $b ) {
+ $this->assertFalse( $a->equals( $b ) );
}
public function testCopyEmptyEquals() {
@@ -267,11 +272,18 @@
$copy->getLabels()->setTextForLanguage( 'en', 'Bar' );
$copy->getDescriptions()->setTextForLanguage( 'en', 'Bar' );
$copy->getStatements()->addNewStatement( new
PropertyNoValueSnak( 24 ) );
+ $copy->getStatements()->getFirstStatementWithGuid( null
)->setRank(
+ Statement::RANK_DEPRECATED
+ );
- $this->assertEquals( 'M1',
$mediaInfo->getId()->getSerialization() );
- $this->assertEquals( 'Foo',
$mediaInfo->getLabels()->getByLanguage( 'en' )->getText() );
- $this->assertEquals( 'Foo',
$mediaInfo->getDescriptions()->getByLanguage( 'en' )->getText() );
- $this->assertEquals( 1, $mediaInfo->getStatements()->count() );
+ $this->assertSame( 'M1',
$mediaInfo->getId()->getSerialization() );
+ $this->assertSame( 'Foo',
$mediaInfo->getLabels()->getByLanguage( 'en' )->getText() );
+ $this->assertSame( 'Foo',
$mediaInfo->getDescriptions()->getByLanguage( 'en' )->getText() );
+ $this->assertCount( 1, $mediaInfo->getStatements() );
+ $this->assertSame(
+ Statement::RANK_NORMAL,
+ $mediaInfo->getStatements()->getFirstStatementWithGuid(
null )->getRank()
+ );
}
}
diff --git
a/tests/phpunit/unit/DatatModel/Serialization/MediaInfoSerializerTest.php
b/tests/phpunit/unit/DatatModel/Serialization/MediaInfoSerializerTest.php
index 76a49d1..f8bda28 100644
--- a/tests/phpunit/unit/DatatModel/Serialization/MediaInfoSerializerTest.php
+++ b/tests/phpunit/unit/DatatModel/Serialization/MediaInfoSerializerTest.php
@@ -110,7 +110,17 @@
public function testSerialize( $object, $serialization ) {
$serializer = $this->newSerializer();
- $this->assertEquals( $serialization, $serializer->serialize(
$object ) );
+ $this->assertSame( $serialization, $serializer->serialize(
$object ) );
+ }
+
+ public function testSerializationOrder() {
+ $mediaInfo = new MediaInfo( new MediaInfoId( 'M1' ) );
+ $serialization = $this->newSerializer()->serialize( $mediaInfo
);
+
+ $this->assertSame(
+ array( 'type', 'id', 'labels', 'descriptions',
'statements' ),
+ array_keys( $serialization )
+ );
}
/**
diff --git a/tests/phpunit/unit/WikibaseMediaInfoHooksTest.php
b/tests/phpunit/unit/WikibaseMediaInfoHooksTest.php
index 050b885..013ee5b 100644
--- a/tests/phpunit/unit/WikibaseMediaInfoHooksTest.php
+++ b/tests/phpunit/unit/WikibaseMediaInfoHooksTest.php
@@ -23,8 +23,8 @@
$paths = [ 'foo' ];
WikibaseMediaInfoHooks::onUnitTestsList( $paths );
- $this->assertEquals( 'foo', $paths[0] );
- $this->assertEquals( realpath( __DIR__ . '/../' ), realpath(
$paths[1] ) );
+ $this->assertSame( 'foo', $paths[0] );
+ $this->assertSame( realpath( __DIR__ . '/../' ), realpath(
$paths[1] ) );
}
public function testOnWikibaseRepoEntityTypes() {
@@ -35,7 +35,7 @@
WikibaseMediaInfoHooks::onWikibaseRepoEntityTypes(
$entityTypeDefinitions );
$this->assertArrayHasKey( 'item', $entityTypeDefinitions );
- $this->assertEquals( [ 'foo', 'bar' ],
$entityTypeDefinitions['item'] );
+ $this->assertSame( [ 'foo', 'bar' ],
$entityTypeDefinitions['item'] );
$this->assertArrayHasKey( 'mediainfo', $entityTypeDefinitions );
$this->assertSerializerFactoryCallback(
$entityTypeDefinitions['mediainfo'] );
@@ -50,7 +50,7 @@
WikibaseMediaInfoHooks::onWikibaseClientEntityTypes(
$entityTypeDefinitions );
$this->assertArrayHasKey( 'item', $entityTypeDefinitions );
- $this->assertEquals( [ 'foo', 'bar' ],
$entityTypeDefinitions['item'] );
+ $this->assertSame( [ 'foo', 'bar' ],
$entityTypeDefinitions['item'] );
$this->assertArrayHasKey( 'mediainfo', $entityTypeDefinitions );
$this->assertSerializerFactoryCallback(
$entityTypeDefinitions['mediainfo'] );
--
To view, visit https://gerrit.wikimedia.org/r/275551
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6f6e75ebb97b3b78de09a00ff61bae93dcfc728b
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/WikibaseMediaInfo
Gerrit-Branch: master
Gerrit-Owner: Thiemo Mättig (WMDE) <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
