Bug 41719: Add rendering test coverage for Labels and Patroncards (rev 2) - #1
Merged
Conversation
Extends t/db_dependent/Labels/t_Label.t with subtests for: - draw_label_text() layout math (text_llx/text_lly pinned to the _BIB geometry contract, downward text flow) - draw_guide_box() PDF stream output - All supported barcode types (CODE39, CODE39MOD, CODE39MOD10, COOP2OF5, INDUSTRIAL2OF5, EAN13), asserting no barcode-generation warning fires per type (Label.pm converts generation failures to warns, so absence-of-death alone cannot detect breakage) - create_label() printing type orchestration (BIB/BAR/BIBBAR/BARBIB), including the geometric contract distinguishing BIBBAR from BARBIB Creates t/db_dependent/Patroncards/t_Patroncard.t with subtests for: - draw_guide_box(), draw_guide_grid(), draw_text() - draw_barcode() across all five dispatched types; the CODE39MOD and CODE39MOD10 rows fail until Bug 43095 lands (missing Algorithm::CheckDigits import in Patroncard.pm, surfaced by this coverage work) - draw_image() - End-to-end PDF output, asserting Koha-originated content (text blocks at layout-computed coordinates, guide-box rectangle) rather than only the PDF header PDF::Reuse emits unconditionally Both files skip cleanly with a clear message when PDF::Reuse < 0.43 is installed (Bug 41717), instead of failing with opaque internals. A shared capture_pdf_end() helper centralises the End() output capture and dies on End() failure instead of swallowing it. Known Bug 41718 and PDF::Reuse prJpeg warnings are filtered against their exact observed text; unexpected warnings are forwarded to Test::NoWarnings. Test plan: 1) Apply Bug 41717 (or cpanm PDF::Reuse@0.43 PDF::Reuse::Barcode@0.09) 2) prove t/db_dependent/Labels/t_Label.t -- all tests pass 3) prove t/db_dependent/Patroncards/t_Patroncard.t -- draw_barcode() subtest fails on CODE39MOD/CODE39MOD10 with "Undefined subroutine ... CheckDigits", demonstrating Bug 43095 4) Apply Bug 43095 and re-run step 3 -- all tests pass AI Assistance: Claude Opus 4.5 drafted the original test code and debugged a PDF::Reuse issue (GitHub cnighswonger/PDF-Reuse#24, fixed in 0.43). Claude Fable 5 (Anthropic) reviewed the first revision, identified tautological assertions, unexercised checksum paths, and the latent Bug 43095 crash, and drafted the revised tests. OpenAI Codex reviewed the revision (helper error propagation, warning-filter precision). Human author directed the approach, rejected an initial implementation that merely exercised PDF::Reuse rather than Koha code, chose to surface rather than skip the Bug 43095 failure, reviewed all changes, and verified test behavior. Assisted-by: Claude Opus 4.5 (Anthropic) Assisted-by: Claude Fable 5 (Anthropic) Assisted-by: Codex (OpenAI)
|
(Automated Close) Please do not file pull requests here, this repo is merely a clone of https://git.koha-community.org/Koha-community/Koha and we use Bugzilla at https://bugs.koha-community.org to submit patches to its codebase.\n\nSee https://wiki.koha-community.org/wiki/Submitting_A_Patch for more information. Note that you can use to easily set-up a full development environment, which includes the command to easily push your patches with a single command, instead of manually fiddling with patch files!\n\nIf you have any more questions or need help, do not hesitate to ask in our Mattermost chat: https://chat.koha-community.org. Thank you. |
cnighswonger
approved these changes
Jul 18, 2026
cnighswonger
left a comment
cnighswonger
left a comment
Owner
There was a problem hiding this comment.
Fixes the case for a document where every Koha draw method died on its first line and yet renders a 2/2 pass.
koha-code-agent
Bot
added
tests
cnighswonger
added
approved-by-lead
koha-code-agent
Bot
added
attached-to-bugzilla
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Revision 2 of the Bug 41719 test-coverage patch (Bugzilla: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41719), addressing the Fable 5 round-1 review (REQUEST_CHANGES) and the Codex round-2 review (APPROVE_WITH_NITS, both nits addressed).
Target commit for Bugzilla attachment after merge:
a7d4df8f4a.Reasoning
Warn-assertions instead of died-checks for Label barcode types.
C4::Labels::Label::barcode()converts everyPDF::Reuse::Barcode::*failure into awarn(Label.pm:556–609), sook(eval { create_label() })can never fail for a barcode reason. Considered slurping the PDF and matching per-type graphic streams instead; rejected because the stream shapes differ per barcode library version, whilewarnings_are { ... } []pins the exact breakage signal the module actually emits.Content assertions instead of header checks for end-to-end output.
PDF::Reuse::prEnd()emits a valid%PDF-header and non-zero file even when no Koha draw method ran, so file-size/header checks are tautologies (this was the round-1 blocker, and the pattern the first draft was rejected for). The revision asserts Koha-originated content. Considered matching the borrower surname literally; rejected because PDF::Reuse's TTFont path CID-encodes text — the assertion instead pins aBT/Tj/ETtext block at the deterministic layout Y (116 = card lly 36 + text-attr lly 80), which only appears ifdraw_textreachedprTextwith non-degenerate layout state.Surfacing rather than skipping the Bug 43095 crash. The two new CODE39MOD/CODE39MOD10 patroncard rows fail with
Undefined subroutine ...CheckDigits(missinguse Algorithm::CheckDigitsimport in Patroncard.pm — filed as Bug 43095, now blocking 41719). Considered TODO-marking them so the suite stays green; rejected per reviewer guidance and operator decision — a coverage patch that hides the defect it uncovered defeats its purpose. The test plan documents the expected failure and its resolution order.Warning filters are exact-match and forwarding. Round-1 flagged blanket
$SIG{__WARN__} = sub {}suppression as blinding Test::NoWarnings. The replacement filters match the empirically captured warning text (Bug 41718 font-lookup warnings; PDF::Reuse$iColorType) and forward everything else to the previous handler directly — perl disables__WARN__handlers inside a__WARN__handler, so a plain re-warn would silently bypass Test::NoWarnings (this also satisfies the repo pre-commit hook's no-warnrule).Deliberately not done: extracting the duplicated
capture_pdf_end()helper into shared test infrastructure (two call-site files don't justify it — Codex concurred); fixing Bug 43095 in this patch (one bug = one scope); asserting on CID-decoded text content (fragile against font-mapping changes).Review history
Verification
Run in koha-testing-docker (PDF::Reuse 0.43 / PDF::Reuse::Barcode 0.09 via cpanm; ktd image ships 0.39):
t/db_dependent/Labels/t_Label.t— 11/11 passt/db_dependent/Patroncards/t_Patroncard.t— all subtests pass except the two intended Bug 43095 rows in draw_barcode()skip_all) when PDF::Reuse < 0.43 — addresses the failure Owen hit in Bugzilla comment 2.perltidyrc; pre-commit hook passesBugzilla hand-off
After merge: attach
a7d4df8f4ato Bug 41719 via git-bz (obsoleting attachment 192076), reply to comment 2, adapt this Reasoning section into the attachment comment.