search

ingenerator / behat-tableassert

Assertions for all sorts of tabular data in behat
BSD 3-Clause "New" or "Revised" License
5 stars 5 forks source link

Time element breaks parsing #15

Closed RoSk0 closed 2 years ago

RoSk0 commented 2 years ago

Table that I need to test contains time element and DOMDocument::loadHTML() fails to parse it.

Also, I allowed myself to add a small improvement in the first commit - used comments as names for datasets. This is recommended to improve readability when tests fail.

RoSk0 commented 2 years ago

Force pushed to fix expected data.

Expect test to fail with:

1) test\Ingenerator\BehatTableAssert\TableParser\HTML\HTMLStringTableParserTest::test_it_parses_table_node_from_valid_html_table with data set "Table with a "time" element" ('<table><thead><tr><td>Header<...table>', array(array('Header', 'Date'), array('Cell1', '30 Aug 2016')))
InvalidArgumentException: Invalid HTML string:
 #801:Tag time invalid (@1:229)
acoulton commented 2 years ago

@RoSk0 that's a brilliant addition with the dataset names, I'd never noticed phpunit had that feature, will help us a lot!

Unfortunately I don't see a way to solve this without refactoring the library to use a third-party html5 parser. Realistically, we don't have time to do that from our end.

If you would like to work on that I'd consider merging it - it might be worth looking at masterminds/html5 which I've seen people using...

RoSk0 commented 2 years ago

Surprisingly I was working on a proof of concept using it.

And here you go - all green locally!

acoulton commented 2 years ago

That's fantastic, thanks.

The only thing I've had to do is bump the minimum masterminds/html5 version to 2.7.5 as it looks like they have issues on 8.1 before that.

RoSk0 commented 2 years ago

Interesting ...

Tests are failing because they are using the lowest versions . Locally I had 2.7.6 installed by the constraint from composer.json.

I would recommend adding additional test runs to the matrix, with the --prefer-lowest instruction , to pick up upcoming issues.

acoulton commented 2 years ago

@RoSk0 as per #19 we're already covering both --lowest and default, so the runs with 2.7.6 passed but 8.1:lowest failed until I bumped the minimum requirement to 2.7.5. All good now.