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

Implement a basic framework for output tests #2510

Merged
merged 5 commits into from Mar 14, 2023

Conversation

Mellthas
Copy link
Member

@Mellthas Mellthas commented Aug 3, 2021

This implements a very basic framework for automatically testing the final output of different documents, to help with finding and fixing regressions, and documenting expected rendering. It uses the GD backend for automatic tests, because comparing the images was simple to implement. This shows several inconsistencies between the CPDF and GD backend, but I think it should still be helpful. PDFs are included as reference for manual checking. I am not too happy about the name OutputTest, maybe someone has a better idea, or maybe it is okay. In relation to the debug helper at https://eclecticgeek.com/dompdf/debug.php, the test cases here are intended to be more constrained and specific, covering edge cases and documenting fixed bugs.

Reference images and PDFs can be updated via the helper script tests/update-reference-output.php. When running the tests, the output of all failed test cases is saved to the tmp directory, so the differences to the reference rendering can be easily checked manually.

If have initially included several test cases that are all broken with the current code, but will be fixed by different PRs (and they have helped greatly in judging the code changes). The font is specified in a way that mimics the rendering of the GD backend as close as possible, as that does only have one fixed font without support for different styles, weights, or letter or word spacing.

Fixed by #2414:

  • inline/letter-spacing

Fixed by #2499:

  • inline/extended-parent-width

Fixed by #2506:

  • page-break/blocks-whitespace
  • page-break/non-empty-first-page

Fixed by #2507:

  • page-break/orphans

Fixed by PR #2497:

  • page-break/inline-element-1
  • page-break/inline-element-2
  • page-break/inline-element-3
  • page-break/inline-block-1
  • page-break/inline-block-2
  • page-break/inline-block-3
  • page-break/paragraph
  • inline/inline-block-width
  • float/float-width
  • table/block-in-table
  • text-align/inline-elements (not completely, but the rendering is improved)

Fixed by #2505:

  • text-align/inline-block (except for the vertical alignment)

Fixed by #2500 (together with #2497):

  • position-relative/block
  • position-relative/inline
  • position-relative/inline-block
  • position-relative/inline-nested
  • position-relative/nested

Fixed by #2542 and #2543:

  • text-align/center
  • text-align/justify
  • text-align/right

The reference rendering reflects the current state and needs to be updated once the different changes are merged.

@bsweeney
Copy link
Member

bsweeney commented Sep 2, 2021

This is very, very nice. Comparing renderings against a reference is one of the things I've wanted to add to the unit tests.

@bsweeney
Copy link
Member

bsweeney commented Sep 3, 2021

My renderings are failing on two different systems, one Windows and one Mac. The rendering on those systems appear to be similar, though I only did a quick visual inspection. The primary culprit seems to be line-height.

Perhaps we should specify a value for most of the settings to ensure a more consistent configuration? Also, what do you think about using the Ahem font developed for use with the CSS conformance suite: https://www.w3.org/Style/CSS/Test/Fonts/

@Mellthas
Copy link
Member Author

Mellthas commented Sep 3, 2021

Okay, I’ll have to take a closer look, have only tested this on a Linux system so far. Can you attach (one of) the failing renders?

The Ahem font looks interesting, though it is probably not very useful as long as the GD back end does not have support for different fonts. Maybe an option is to add support for just that font for use in the tests, but haven't looked into that.

@bsweeney
Copy link
Member

bsweeney commented Sep 3, 2021

Font support in GD seems OK to me, though I haven't tested on a lot of systems.

I rendered the following html on my two system:

<link href='http://fonts.googleapis.com/css?family=Give+You+Glory&amp;v2' rel='stylesheet' type='text/css'>
<style> @page { margin: 0px; size: 350px 400px; } </style>
<p style="font-family: 'Give You Glory'; font-size: 30pt;">
  Grumpy wizards make toxic brew for the evil Queen and Jack
</p>

and it looks OK:

glory

@bsweeney
Copy link
Member

bsweeney commented Sep 3, 2021

Here's what I got for float-width.html:

float-width p1 fail

@Mellthas
Copy link
Member Author

Mellthas commented Sep 3, 2021

:-D Oh my, I think I simply assumed it had no support for different fonts, because font-family: serif didn't work …

@bsweeney
Copy link
Member

bsweeney commented Sep 3, 2021

I had no idea serif wasn't working!

@Mellthas
Copy link
Member Author

Mellthas commented Sep 3, 2021

It seems specifying any of the generic font families results in DejaVu Sans being used, but without support for font weights and styles.

@Mellthas
Copy link
Member Author

Mellthas commented Sep 3, 2021

I updated the float-width test to specify the font explicitly. Does that make your rendering match the updated reference?

@Mellthas
Copy link
Member Author

The line-height issue seems to be exhibited with DejaVu Sans. In the image you posted, @bsweeney, the line height seems to be calculated correctly: With a font size of 12pt and a line height of 1.2 (the default values), the actual line height calculates to 14.4 pt, which would fit 13, almost 14 lines into the box. With the CPDF back end the actual line height is much larger, even larger than using the GD back end on my system, which is still too large. Here is the output of text-align/center on my system which specifies a line-height of 14pt, which should fit 14 lines onto one page:

GD
center p1
center p2

CPDF
center.pdf

But as you can see, the GD output only fits 12, the CPDF one only 11 lines. Switching to the default sans-serif font, 14 lines fit using the CPDF back end, as expected with a line height of 14pt.

@Mellthas
Copy link
Member Author

Apart from the line-height issue there also seem to be differences wrt. kerning (and probably anti-aliasing), which could also change between different PHP versions and the (system) libraries behind. I think checking for a pixel-perfect replication is a bit too simplistic for the automatic tests. Even allowing for a small margin of error might not be enough, as the layout might change due to the text width being slightly off.

@Mellthas
Copy link
Member Author

GD rendering is significantly improved with the recent rounding fixes from master. PNG and PDF output are much closer now.

@johanadivare
Copy link

@johanadivare I will look at this next, just want to finish up the on some small updates to selector parsing.

@Mellthas can you give a update?

@johanadivare
Copy link

@johanadivare I will look at this next, just want to finish up the on some small updates to selector parsing.

@Mellthas can you give a update?

@Mellthas if you dont have time maybe someone else can continue now its stuck for a while

@Mellthas
Copy link
Member Author

Mellthas commented Mar 5, 2023

@johanadivare I am sorry for the long silence. I will see what I can do in the coming days.

@Mellthas
Copy link
Member Author

Mellthas commented Mar 6, 2023

Okay, here we are. I reworked the tests to compare the PDF output produced by the CPDF back end with the help of Ghostscript. The PDF files are converted to images for comparison during testing only now. This should avoid any problems with differing font rendering. @johanadivare This should also avoid the need to use a Docker setup. Ghostscript is needed to run these tests now, and if it is not available, the tests are skipped.

@IonBazan Thanks for the review! I have incorporated your review points, and moved the fixture files as suggested. I also moved the update-reference-output.php helper to the bin directory.

@Mellthas
Copy link
Member Author

Mellthas commented Mar 6, 2023

The image comparison uses Imagick if it is available now. It should be a bit faster than using the GD functions. The Imagick::compareImages() method also returns an image visualizing the differences, which could be written to a file on failure.

Copy link
Contributor

@IonBazan IonBazan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for picking it up and wrapping it up again.

Copy link

@johanadivare johanadivare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good

It would also be nice if there was a way for projects that use this package to use the test in there own project by passing reference files and compare files. But that might be better for new pull request

tests/OutputTest/OutputTest.php Outdated Show resolved Hide resolved
tests/OutputTest/OutputTest.php Outdated Show resolved Hide resolved
tests/OutputTest/OutputTest.php Outdated Show resolved Hide resolved
tests/OutputTest/OutputTest.php Outdated Show resolved Hide resolved
@Mellthas Mellthas force-pushed the output-tests branch 3 times, most recently from 45e1ae8 to a60cb4c Compare March 7, 2023 23:59
@Mellthas Mellthas force-pushed the output-tests branch 4 times, most recently from 38bffa7 to 3fb02d3 Compare March 10, 2023 00:16
Reference files can be updated via the helper script
`bin/update-reference-output.php`. When running the tests, the output
of all failed test cases is saved to the local `tmp` directory, so the
differences to the reference rendering can be easily checked manually.
To ensure that output tests are run successfully on CI; they are skipped
if Ghostscript is not installed.
Non-exhaustive, mostly verifying the recent changes to the rendering
logic.
@Mellthas
Copy link
Member Author

Okay, I have looked over the initial set of test cases and added some new ones for border rendering. I think this is good to go now. Further enhancements can be done in new PRs.

@Mellthas Mellthas requested a review from bsweeney March 12, 2023 16:52
@Mellthas Mellthas merged commit 595a7a6 into dompdf:master Mar 14, 2023
@Mellthas Mellthas deleted the output-tests branch March 14, 2023 22:30
@Mellthas Mellthas added this to the 2.0.4 milestone Mar 23, 2023
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 this pull request may close these issues.

Add a functional test that compares generated PDF outputs
6 participants