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

TST: Add test for correct rendering of watermarks #2130

Merged
merged 7 commits into from
Sep 3, 2023

Conversation

stefan6419846
Copy link
Collaborator

@stefan6419846 stefan6419846 commented Aug 30, 2023

This adds a test for the correct rendering of watermarked files.

Closes #2112

@codecov
Copy link

codecov bot commented Aug 30, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.02% 🎉

Comparison is base (2cd129c) 94.13% compared to head (b31a53a) 94.16%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2130      +/-   ##
==========================================
+ Coverage   94.13%   94.16%   +0.02%     
==========================================
  Files          41       41              
  Lines        7467     7467              
  Branches     1474     1474              
==========================================
+ Hits         7029     7031       +2     
+ Misses        273      271       -2     
  Partials      165      165              

see 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@pubpub-zz pubpub-zz left a comment

Choose a reason for hiding this comment

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

This test should not prevent passing tests if gs is not installed

tests/test_writer.py Outdated Show resolved Hide resolved
tests/test_writer.py Outdated Show resolved Hide resolved
@stefan6419846
Copy link
Collaborator Author

Just for the record: I currently get a similarity value of 0.765599726490022 when running the check against the original page. I am not sure whether we should verify that this value always stays below the limit imposed for the watermark variant.

@pubpub-zz
Copy link
Collaborator

Just for the record: I currently get a similarity value of 0.765599726490022 when running the check against the original page. I am not sure whether we should verify that this value always stays below the limit imposed for the watermark variant.

What is the results when you compare without the fix ? I think comparing with .5 would be required. However I'm surprised you are getting such a low comparing result...

@stefan6419846
Copy link
Collaborator Author

The current comparison of the expected and actual result yields 100 % similarity, which I do not really consider surprising for PNGs where the general rendering artifacts are mostly non-existent (compared to DCTs for JPEGs). The remaining 5 % in the check allow for some general flexibility.

I do not really think that 76.5 % similarity between the original and watermarked version is low due to the fact that the test basically adds a background image/pattern to the page. The value should be the same as in the errorneous state due to the watermark not being visible in this case as well, but I have not tested this again.

@pubpub-zz
Copy link
Collaborator

What I meant is that without the fix the page is displayed blank. the comparison should return about 0 but the test will still pass. Am I wrong ?

@stefan6419846
Copy link
Collaborator Author

What I meant is that without the fix the page is displayed blank. the comparison should return about 0 but the test will still pass. Am I wrong ?

Why would the test still pass when there would be 0 % similarity, but we set a similarity gate of >= 95 %?

Nevertheless, the blank case only occurs for already blank input files, which is what I provided as initial reproducing example on my bug report as white pages are uncritical regarding privacy. Citing from my initial report at #2112:

only show a blank page (or the non-watermarked version if the input file would not have had a blank page).

This means that the document would (mostly) be rendered the same as the input document, sometimes (like in Adobe Acrobat) with some warnings regarding the issues.

@pubpub-zz
Copy link
Collaborator

I have to appologize again: I did not see the assertion test. This is all good!👏

@MartinThoma MartinThoma added the soon PRs that are almost ready to be merged, issues that get solved pretty soon label Sep 2, 2023
MartinThoma added a commit that referenced this pull request Sep 3, 2023
This was originally added by stefan on #2130

This allows anyone to manually run the workflow on their fork if desired with any branch.

See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_dispatch

Co-authored-by: stefan6419846 <96178532+stefan6419846@users.noreply.github.com>
MartinThoma added a commit that referenced this pull request Sep 3, 2023
This was originally added by stefan on #2130

This allows anyone to manually run the workflow on their fork if desired with any branch.

See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_dispatch

Co-authored-by: stefan6419846 <96178532+stefan6419846@users.noreply.github.com>
@MartinThoma
Copy link
Member

I've added the workflow_dispatch via #2145. Although I don't quite see the need for it, I also see no harm. If it's useful for you, I'm happy to add it.

I wanted to have this in a separate PR as it's completely unrelated to the core changes in here.

@MartinThoma
Copy link
Member

@stefan6419846 If you're ok with it, I would add those three files:

to https://github.com/py-pdf/sample-files . That is better than having it just on Github as its directly cloned.

Do you own the copyright on those files (aka: did you create them)?

Is it OK for you if I add them to the sample-files repository?

tests/test_writer.py Outdated Show resolved Hide resolved
@MartinThoma MartinThoma merged commit b8d3bea into py-pdf:main Sep 3, 2023
14 checks passed
@MartinThoma
Copy link
Member

Thank you for providing this test 🙏

I'll check what I have in my private repository; hopefully tomorrow. My test is for sure different ... maybe there is value in having both ways 🤔

@MartinThoma MartinThoma removed the soon PRs that are almost ready to be merged, issues that get solved pretty soon label Sep 3, 2023
MartinThoma added a commit that referenced this pull request Sep 3, 2023
## What's new

### Bug Fixes (BUG)
-  Cope with missing /I in articles (#2134)
-  Fix image look-up table in EncodedStreamObject (#2128)
-  remove_images not operating in sub level forms (#2133)

### Robustness (ROB)
-  Cope with damaged PDF (#2129)

### Documentation (DOC)
-  How to take ownership (#2123)

### Developer Experience (DEV)
-  Download PDFs before executing the tests to not run into timeouts (#2143)
-  Add workflow_dispatch to CI (#2145)
-  Automatically create release message / tag message (#2127)
-  Ensure the REL commit message is consistently created (#2126)

### Testing (TST)
-  Add test for correct rendering of watermarks (#2130)

[Full Changelog](3.15.4...3.15.5)
@stefan6419846
Copy link
Collaborator Author

Do you own the copyright on those files (aka: did you create them)?

No, these are the ones added by #1952 and have been uploaded in #1951 (comment) by @ucomru.

@stefan6419846 stefan6419846 deleted the watermarking branch September 4, 2023 15:08
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.

Mostly broken watermarking behaviour
3 participants