search

mikehaertl / phpwkhtmltopdf

A slim PHP wrapper around wkhtmltopdf with an easy to use and clean OOP interface
MIT License
1.6k stars 238 forks source link

Tests started to fail #339

Closed timwsuqld closed 4 years ago

timwsuqld commented 4 years ago

I first noticed this on Ubuntu 18.04, but luckily for me, they fail in Travis as well. It seems something has changed how the quoting works, and so parts of the test now fail due to missing quotes around arguments. https://travis-ci.org/mikehaertl/phpwkhtmltopdf/jobs/632115902?utm_medium=notification&utm_source=github_status

5) PdfTest::testCanAddCoverFromUrl

Failed asserting that two strings are equal.

--- Expected

+++ Actual

@@ @@

-'/usr/local/bin/wkhtmltopdf cover 'http://www.google.com/robots.txt' '/tmp/tmp_wkhtmlto_pdf_gez3j9.pdf''

+'/usr/local/bin/wkhtmltopdf 'cover' 'http://www.google.com/robots.txt' '/tmp/tmp_wkhtmlto_pdf_gez3j9.pdf''

I've no idea what's changed as our CI environment only installs security updates, so best guess is some security update in PHP is to blame for this.

mikehaertl commented 4 years ago

This is because of a change in my php-shellcommand: https://github.com/mikehaertl/php-shellcommand/issues/44.

I forgot to update the tests here so thanks a lot for your fix.

timwsuqld commented 4 years ago

@mikehaertl Just wondering when you'll tag another release so these fixes are pushed? Thanks

mikehaertl commented 4 years ago

@timwsuqld There are no new features or fixes so far only fixes for the test setup. I think this should only be relevant for developers that check out the repo anyway. Or am i missing something? Maybe you could explain how this would help you?

timwsuqld commented 4 years ago

@mikehaertl We actually run the phpwkhtmltopdf tests as part of our CI process. It was one of the ways we ensured that our environment (docker) correctly produced PDF's and to ensure that updates to wkhtmltopdf or phpwkhtmltopdf didn't break something.

Looking through the commits, I can see that you're right about a release not introducing anything new other than for developers. For now, I've just got the tests disabled in CI and an internal issue to enable them after the next phpwkhtmltopdf release. If it becomes a pain, I'll point composer at the master branch instead and sort it that way. I (wrongly) assumed that the other merge requests since the last tag included something for users, and not just developers.

Thanks

mikehaertl commented 4 years ago

Ok, created 2.4.2 because another release doesn't really hurt either.

timwsuqld commented 4 years ago

@mikehaertl Thanks heaps for that. You're a champ