Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

content: Remove unclear assertEquals() on Status objects

Browse source 
We don't usually compare object serializations with assertEquals as
they tend to make it difficult to know what is and isn't being asserted,
giving the illusion that it checks "everything" but can in some cases
be closer to "too much" or "nothing".

Actual logic:
* https://github.com/sebastianbergmann/phpunit/blob/9.6.17/src/Framework/Assert.php#L330
* https://github.com/sebastianbergmann/phpunit/blob/9.6.17/src/Framework/Constraint/Equality/IsEqual.php
* https://github.com/sebastianbergmann/comparator/blob/5.0.1/src/ObjectComparator.php
* https://github.com/sebastianbergmann/exporter/blob/5.1.2/src/Exporter.php

This means sub classes or value objects are needlessly discarded,
as well as potentially irrelevant state in the object is becoming
part of the test.

It is not a surprise then, that these assertions are not comparing
against any particular desired outcome, but are literally a copy-paste
of the source code, including functions that have no effect in testing
such as `ContentHandler::getLocalizedName( 'testing' )` === 'testing',
and untested expressions like `$wikipage->getTitle()->getPrefixedText()`
instead of an explicit expectation.

Replace this with assertStatusError and limit the assertion to
being !isOK, and error using the specified interface message key.

Testing that the correct parameters are passed is imho not a part
of this test, since the test isn't actually validating it to be
correct or sensible in terms of message contents and parsed outcome,
it is only verifying that we have correctly copied the source, which
still needs the same review as it otherwise would.

Change-Id: I8c7a660489e9000f9790f8d69478a05ad8c446b6
This commit is contained in:
Timo Tijhof 2024-03-11 15:08:41 -07:00 committed by D3r1ck01
parent 3fabbf33b4
commit 3dad835561
1 changed files with 7 additions and 23 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

30
tests/phpunit/includes/content/ContentModelChangeTest.php
View file

@ -64,10 +64,7 @@ class ContentModelChangeTest extends MediaWikiIntegrationTestCase {
__METHOD__ . ' comment',
false
);
$this->assertEquals(
Status::newFatal( 'apierror-nochanges' ),
$status
);
$this->assertStatusError( 'apierror-nochanges', $status );
}
/**
@ -139,10 +136,7 @@ class ContentModelChangeTest extends MediaWikiIntegrationTestCase {
__METHOD__ . ' comment',
false
);
$this->assertEquals(
Status::newFatal( $expectedMessage ),
$status
);
$this->assertStatusError( $expectedMessage, $status );
}
public static function provideTestEditFilterMergedContent() {
@ -188,14 +182,7 @@ class ContentModelChangeTest extends MediaWikiIntegrationTestCase {
__METHOD__ . ' comment',
false
);
$this->assertEquals(
Status::newFatal(
'apierror-changecontentmodel-cannotbeused',
'plain text',
Message::plaintextParam( $wikipage->getTitle()->getPrefixedText() )
),
$status
);
$this->assertStatusError( 'apierror-changecontentmodel-cannotbeused', $status );
}
/**
@ -230,11 +217,8 @@ class ContentModelChangeTest extends MediaWikiIntegrationTestCase {
__METHOD__ . ' comment',
false
);
$this->assertEquals(
Status::newFatal(
'apierror-changecontentmodel-nodirectediting',
ContentHandler::getLocalizedName( 'testing' )
),
$this->assertStatusError(
'apierror-changecontentmodel-nodirectediting',
$status
);
}
@ -251,8 +235,8 @@ class ContentModelChangeTest extends MediaWikiIntegrationTestCase {
'text'
);
$status = $change->setTags( [ 'edit content model tag' ] );
$this->assertEquals(
Status::newFatal( 'tags-apply-no-permission' ),
$this->assertStatusError(
'tags-apply-no-permission',
$status
);
}