PhilterPaper / Perl-PDF-Builder

Extended version of the popular PDF::API2 Perl-based PDF library for creating, reading, and modifying PDF documents
https://www.catskilltech.com/FreeSW/product/PDF%2DBuilder/title/PDF%3A%3ABuilder/freeSW_full
Other
6 stars 7 forks source link

Improve tests for convert, tiffcp and Graphics::TIFF #129

Closed carygravel closed 3 years ago

carygravel commented 3 years ago

This removes the ugly shell-dependent code and make things pure Perl. It also works. I've tested with and without convert and Graphics::TIFF.

PhilterPaper commented 3 years ago

Did you take a look at the tiff.t I sent you in the other PR discussion? I already took care of determining if Graphics::TIFF was available ($has_GT variable set in the first test), unless you found it doesn't work on Linux. I also updated the skip messages and enabled test 9. I put the Graphics::TIFF check as a separate skip with its own message, but wasn't sure which tests need GT, as I can't run this stuff on Windows. If $OSNAME is no longer used, you should remove use English. Finally, are Try::Tiny, IPC::Cmd, and any other new test prereq packages part of core at least as far back as 5.20? I don't want to make users have to install new non-core packages if at all possible.

carygravel commented 3 years ago

IPC::Cmd is core in Perl 5.20

I've incorporated your ideas from PR128 and updated this one.

PhilterPaper commented 3 years ago

I have found a few issues with your latest tiff.t:

  1. Windows has its own "convert" utility, which rewrites the file system! We don't want people accidentally running that one when can_run finds it! Therefore, I think it would be safer to continue checking the OS to be 'linux' (or, that the OS is not Windows). can_run could still be used after it's checked that the OS is Linux (or you can go back to test running convert and tiffcp to /dev/null, and omit IPC::Cmd).
  2. I see Try::Tiny is still in the Makefile.PL. It's not core. Are you still using it anywhere?
  3. You left out the $has_GT = 0; line. Are you relying on an undefined $has_GT being treated as 0? I don't like doing that, as someone may tighten up error checking in the future, and break a lot of things.
  4. What is the story on Test 9 (alpha)? Can you tell if the problem is in PDF::Builder, or is convert or tiffcp having problems with Alpha? Unless it's an easy fix, we should probably just continue skipping this test. Is there a chance that it could be fixed in the not too distant future? If NOT, we should consider removing it altogether.
  5. Is Test 10 the only one requiring Graphics::TIFF (test 9 skipped, test 11 doesn't need it?)
  6. You chose not to update the Test 10 skip message. Was that an oversight, or do you have a reason? I see no reason to print out an error message that the user should never see.
  7. I can't find any documentation on 'skip'. Is ...", 1, unless... definitely the same as ...", 1 unless...?

I made a little utility "isCore.pl" to look up whether a package is core or not:

use strict;
use warnings;
use Module::CoreList; 

if (@ARGV != 1) {
    print "isCore.pl name-of-module\nreturns whether it is Core and what was the first release (in Core)\n";
    exit;
}
if (Module::CoreList::is_core($ARGV[0])) {
    print "$ARGV[0] IS a core module\n";
    print "First appeared in Perl ".Module::CoreList->first_release($ARGV[0]); 
} else {
    print "$ARGV[0] is NOT a core module\n";
}
carygravel commented 3 years ago
  1. You can download imagemagick for windows here. It looks as though at least v7 uses magick rather convert.
  2. Good catch. I've removed Try::Tiny.
  3. If an undefined $has_GT is ever not treated as a boolean false by a condition, then many other things than PDF::Builder will break.
  4. The problem is in PDF::Builder, but I don't know immediately how to fix it. The test, however, will never run. It is always skipped. I think the test should stay to remind us that the problem is there.
  5. Yes
  6. I've updated the skip message
  7. That's Perl syntax, not specific to skip. But you are right. I didn't change that deliberately.

Please take another look

PhilterPaper commented 3 years ago
  1. OK, maybe I'll try installing ImageMagick some time later to work on test 9
  2. OK
  3. I guess skip "message", 1 unless boolean; would also take undef == 0 if boolean true? Just as a personal coding style, I don't like to leave things like that. I suspect that I could remove "1 unless" and get the same result.
  4. If you think the problem is in PDF::Builder (convert produces a good, readable TIFF file), and you're sure that Graphics::TIFF (libtiff) should be able to handle this sort of alpha layer, please consider opening a ticket against PDF::Builder. If the problem appears to be in convert or tiffcp, we're stuck.
  5. OK
  6. OK
  7. OK. I will try removing "1 unless" (just a boolean to return 0 or 1) and see what happens (invert test with !).

I have gone ahead and merged your changes. Again, thank you for your work.