[4.0] Fixing Travis #16796
[4.0] Fixing Travis #16796
Conversation
| @@ -92,14 +92,14 @@ protected function tearDown() | |||
| */ | |||
| public function testFetchButton() | |||
| { | |||
| $html = "<button onclick=\"if (document.adminForm.boxchecked.value == 0) { Joomla.renderMessages({'error': [Joomla.JText._('JLIB_HTML_PLEASE_MAKE_A_SELECTION_FROM_THE_LIST')]}) } else { if (confirm('Confirm action?')) { Joomla.submitbutton('article.save'); } }\" class=\"btn btn-sm btn-outline-danger\">\n" | |||
| $html = "<button id=\"toolbar-\" onclick=\"if (document.adminForm.boxchecked.value == 0) { Joomla.renderMessages({'error': [Joomla.JText._('JLIB_HTML_PLEASE_MAKE_A_SELECTION_FROM_THE_LIST')]}) } else { if (confirm('Confirm action?')) { Joomla.submitbutton('article.save'); } }\" class=\"btn btn-sm btn-outline-danger\">\n" | |||
There was a problem hiding this comment.
This ID seems very sad. Is that some weird unit test setup - or is there something weird with the code?
There was a problem hiding this comment.
fetch button appends normally the given name, as there is none in the setup we just append ''; ...
On a site note can we please stop testing exact HTML output our unit tests? This is breaking every simple intended change.. Let's test functionality :)
There was a problem hiding this comment.
Well when a method has no job but to generate HTML, what else is there to test?
There was a problem hiding this comment.
Actually, it shows now an error in the original PR, there is a doubled space generated 😄
There was a problem hiding this comment.
Well when a method has no job but to generate HTML, what else is there to test?
@mbabker To be blunt: Nothing, as it's not functionality.. We test string concat :) If it has functionality / logic we could test the parts that are changed with it..
If we really want to go this way we can use DOMDocument in PHPUnit, to just test if attributes are right..
Sample:
$document = new \DOMDocument($actual);
$element = $document->getElementById('toolbar-');
$this->assertNotNull..
$this->assertRegexp('/btn-outline-danger/', $element->getAttribute('class')); // Can't use assertContains .. in PHPUnit 4.8....What do you think?
This would make the tests way more rubust and you see where the change actually happend if it was by error..
There was a problem hiding this comment.
From a unit test perspective, I don't think we just blow off testing methods whose functionality is to generate an HTML string (also remember that string can be dependent on parameters, so validating parameters get applied correctly can be helpful).
We can do better. We need to do better. I said we need to do better almost a year ago. This doesn't mean that we have to do a character-for-character validation of the generated markup, but the test definitely shouldn't boil down to "did it actually return something" because at that point it's just wasting CPU cycles.
|
Removing the doubled space from the JLayout. Waiting for Travis to see if that fixes all of the issues 😄 |
|
@wilsonge Travis passes now. But it became a bit more than just unit tests. The layouts generated a doubled space since $id already contains the space (" id=foo") |
Fixing Travis error introduced by #16779
Some tests did also have "expected" and "actual" inverted. I've switched those.
I don't understand why
JToolbarButtonConfirmTest::testFetchButtonhas the idid="toolbar-", looks like it is missing a part there. But Travis should (hopefully) pass again.