? ?
avatar mbabker
mbabker
1 Jul 2016

For methods that output HTML and the corresponding tests checking these strings, the tests are flaky and somewhat unstable as an arbitrary whitespace change can cause the test to start failing for no valid reason.

Tests validating HTML should be rewritten in a manner which does not perform explicit string comparisons. This was attempted in some cases with assertTag() but PHPUnit deprecated this feature and removed it in 5.0.

avatar mbabker mbabker - open - 1 Jul 2016
avatar mbabker mbabker - change - 1 Jul 2016
Labels Added: ? ?
avatar brianteeman brianteeman - change - 1 Jul 2016
Category Unit Tests
avatar brianteeman brianteeman - change - 1 Jul 2016
Labels
avatar mbabker
mbabker - comment - 2 Jul 2016
avatar dgt41
dgt41 - comment - 2 Jul 2016

Any suggestions?

avatar dneukirchen
dneukirchen - comment - 7 Jul 2016

In my opinion most of the methods which output html, should use JLayout to support overrides. So in the tests for these methods we should only check if they call JLayout::render with the correct parameters.

Im not a friend of testing templates/layouts, because they change to often. But we could create html output tests for the important parts/elements of the layouts with DomDocument and assertEqualXMLStructure():
https://phpunit.de/manual/5.4/en/appendixes.assertions.html#appendixes.assertions.assertEqualXMLStructure

avatar dgt41
dgt41 - comment - 7 Jul 2016

@dneukirchen for some reason assertXmlStringEqualsXmlFile() failed here: #10995
So not sure that assertEqualXMLStructure() will actually work

avatar dneukirchen
dneukirchen - comment - 7 Jul 2016

In that special case (we only have to check some properties/attributes and values) we could write a test for each attribute/value check and use DomDocument helpers like $this->assertTrue($actual->hasAttribute('disabled')).

avatar dneukirchen
dneukirchen - comment - 7 Jul 2016

Or if you wanna stay with the helper (its not so readable in my opinion. i would prefer the $this->assertTrue($actual->hasAttribute('disabled')) check i mentioned above.)

public function testGetInput($data, $expectedHtml)
{
    $formField = new JFormFieldUrl;
    foreach ($data as $attr => $value)
    {
        TestReflection::setValue($formField, $attr, $value);
    }

    $expected = new DOMDocument;
    $expected->loadXML($expectedHtml);

    $actual = new DOMDocument;
    $actual->loadXML(TestReflection::invoke($formField, 'getInput'));

    $this->assertEqualXMLStructure(
        $expected->firstChild,
        $actual->firstChild,
        true, // check attributes
        'Line:' . __LINE__ . ' The field did not produce the right html'
    );
}
avatar laoneo
laoneo - comment - 8 Jul 2016

In my case assertXmlStringEqualsXmlString did the job. But as soon as you add new attributes which are empty it fails. Perhaps an approach can be to add our own assertXMLStringEquals function which can compare strings and tests empty attributes.

avatar joomla-cms-bot joomla-cms-bot - change - 8 Jul 2016
The description was changed
avatar joomla-cms-bot joomla-cms-bot - edited - 8 Jul 2016
avatar franz-wohlkoenig franz-wohlkoenig - change - 6 Apr 2017
Status New Needs Review
avatar rdeutz rdeutz - assigned - 25 May 17
avatar rdeutz
rdeutz - comment - 25 May 2017

I am not a fan of testing output in unit test, so I would simple drop them or go with David suggestion

the tests for these methods we should only check if they call JLayout::render with the correct parameters.

avatar yvesh
yvesh - comment - 25 May 2017

Agree with Robert here, we just keep fixing them again and again, because of intended output changes with really low value.

avatar mbabker
mbabker - comment - 3 Apr 2018

Tests are gone

avatar mbabker mbabker - change - 3 Apr 2018
Status Needs Review Closed
Closed_Date 0000-00-00 00:00:00 2018-04-03 12:26:41
Closed_By mbabker
avatar mbabker mbabker - close - 3 Apr 2018

Add a Comment

Login with GitHub to post a comment