Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests started to fail #339

Closed
timwsuqld opened this issue Jan 3, 2020 · 6 comments · Fixed by #340
Closed

Tests started to fail #339

timwsuqld opened this issue Jan 3, 2020 · 6 comments · Fixed by #340

Comments

@timwsuqld
Copy link
Contributor

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
Copy link
Owner

This is because of a change in my php-shellcommand: mikehaertl/php-shellcommand#44.

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

@timwsuqld
Copy link
Contributor Author

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

@mikehaertl
Copy link
Owner

@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
Copy link
Contributor Author

@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
Copy link
Owner

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

@timwsuqld
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants