PHPUnit 4.1.5 / assertMatch returns Unexpected end tag : hr

[mwjames]

Tests running on PHPUnit 4.1.4 went without problems [0]:(on the tests that use assertMatch ) but when running the same tests against 4.1.5 that use $this->assertTag( $matcher, $result ); a Unexpected end tag : hr is returned without having changed those specific tests in months.

PHPUnit_Framework_Exception: 
Unexpected end tag : hr

[0] https://travis-ci.org/SemanticMediaWiki/SemanticMediaWiki/jobs/31666571
[1] https://travis-ci.org/SemanticMediaWiki/SemanticMediaWiki/jobs/31739725

[whatthejeff]

I think this is probably related to #1354.

Ping @sun.

[whatthejeff]

Also, might be worth mentioning that assertTag() will be deprecated soon (see: #1292).

[whatthejeff]

@mwjames Would you mind providing us with the HTML you're testing against so that we can build a regression test case?

[mwjames]

Would you mind providing us with the HTML you're testing against so that we can build a regression test case?

While providing the matcher isn't the problem:

$matcher = array(
    'tag' => 'form',
    'descendant' => array(
        'tag' => 'input',
        'attributes' => array( 'name' => 'property', 'value' => 'Foo' )
    )
);

$this->assertTag( $matcher, $result );

Providing the result html itself is an issue as it is generated by MediaWiki's Xml::tags, Html::hidden methods.

EDIT: The html the test run against would look like this.

[sun]

The first failure appears to be in
https://github.com/SemanticMediaWiki/SemanticMediaWiki/blob/master/tests/phpunit/includes/querypages/QueryPageTest.php#L117

It seemingly asserts a basic form with a simple input element. Can you explain where a HR HTML element is involved in that test?

Are these the only tests that are failing? If so, are you sure that the tests are correct?

Unexpected end tag : hr

…sounds as if the test fixture document would contain e.g. </hr> instead of <hr/>, and there might have been updates to either PHP core, any involved XML parsers, or possibly also in PHPUnit, which are now catching the issue and properly exposing it?

In any case, I'm not able to relate this unexpected failure to any recent changes in PHPUnit.

[mwjames]

It seemingly asserts a basic form with a simple input element. Can you explain where a HR HTML element is involved in that test?

The generator uses something like Xml::tags( 'hr', array( 'style' => 'margin-bottom:10px;' ), '' ) (which doesn't help you much since this is MediaWiki specific but the <hr> element is expected to be generated).

Currently, my only concern is that 4.1.4 run went smooth but 4.1.5 doesn't without having changed the source nor the tests.

In any case, I'm not able to relate this unexpected failure to any recent changes in PHPUnit.

I just locally switched from 4.1.5 to 4.1.4 where the issue does not appear (same environment, same installed base) but as soon as I go back to 4.1.5 the error appears.

[whatthejeff]

I just locally switched from 4.1.5 to 4.1.4 where the issue does not appear (same environment, same installed base) but as soon I go back to 4.1.5 the error appears.

Yep, I can confirm this. I'll see what I can find :)

[whatthejeff]

Seems c1bf461 is exposing errors that were hidden before.

[sun]

Oh, yes, indeed. I assumed that was a Windows-only quirk. Looks like it was also hiding away libxml parser errors on non-Windows platforms?

[whatthejeff]

Yeah. Seems we were previously only reporting LIBXML_ERR_FATAL errors. Now we are reporting general errors/warnings because of the new || $message !== ''.

[sun]

So I guess we unknowingly improved error reporting and this "works as designed"…?

Perhaps just the error/exception message in the test result could be a bit more clear?

[whatthejeff]

@sun Let's wait and see what kind of feedback we get from the community / @sebastianbergmann. It is an improvement, but it does break BC because I can't say for sure that this was the way the code was intended to work.

@mwjames The failure is caused by having a closing <hr> tag in <hr style="margin-bottom:10px;"></hr>. There are two easy fixes:

1. Tell the component that generates the HTML that you don't need the </hr>.
2. Tell assertTag that you are testing XML:

$this->assertTag( $matcher, $result, '', false );

[mwjames]

Tell assertTag that you are testing XML:

Using $this->assertTag( $matcher, $result, '', false ); solved the issue for the first failed test but for the second failed test even after changing assertTag it returns with a new PHPUnit_Framework_Exception: Extra content at the end of the document therefore switching to assertContains to avoid more surprises from assertTag.

When removing assertTag from the assert repository, using assertContains is a poor man's replacement and #1292 doesn't give any good alternatives.

[whatthejeff]

@mwjames Sorry for the inconvenience. I think we will probably revert this change (or make it optional) for the next release.

When removing assertTag from the assert repository, using assertContains is a poor man's replacement and #1292 doesn't give any good alternatives.

Yeah, sorry about that. I hope to have a plugin that we can recommend shortly.

[whatthejeff]

@sun what do you think about making this optional?

[whatthejeff]

@mwjames I've started working on an alternative here: https://github.com/phpunit/phpunit-dom-assertions. It's still a work in progress, but it should be available by the time assertTag(), assertSelectEquals(), etc. are officially removed.

[sun]

Adding an optional $strict parameter to the load() method to fail on all errors instead of just "DOM soup" warnings sounds good to me. I've to admit I wasn't aware that the utility method is not only used by the XML configuration loader, but also by assertions. Due to that, the new parameter should probably default to FALSE and only the configuration loader passes TRUE.

[whatthejeff]

@sun Sounds good. Thanks :)

[whatthejeff]

Reopening until we have a regression test :)

[whatthejeff]

Alright, I'll ping @sebastianbergmann to roll a new release for us. Thanks for your patience, @mwjames.

[whatthejeff]

Alright, we've released 4.1.6 which should fix this regression.

[mwjames]

Thanks