testsuite: test the oiiotool 'Channel reordering and padding' doc examples - #5464
linsen458-spec wants to merge 1 commit into
Conversation
|
|
994c08d to
94346c0
Compare
|
I think there might be a misunderstanding about what #3992 is asking for (outlined in more detail in the wiki page linked to on that issue). The goal is:
The idea here is not merely to have tests from the examples represented in the testsuite (though that is one goal, but as a note, we do already test various Now, I think you may be the first to do this for the oiiotool chapter, and I fear that it exposes that I have omitted something you probably needed: While I have testsuite/docs-examples-cpp and testsuite/docs-examples-python, it looks like I never made testsuite/docs-examples-oiiotool that gives you a place to put the code or a full example recipe to follow. I will try to set up a single example the right way, and then you can use that as a template. Will try that as soon as I can. |
|
Ah, thanks for taking the time to explain — that makes sense now. I read it as "recreate the examples as tests", which as you point out still leaves room for the two copies to drift apart. Making them literally the same lines is a much better idea. Happy to redo this once you've got the |
|
I have some fixes for the runtest.py that will make this MUCH easier. Will try to get a PR for it tonight. Thanks for understanding. |
|
I hope it's clear what the application of this change to runtest.py is to your PR here -- you can now easily express exactly the text that somebody would type on the command line (and what you want to appear verbatim in the docs) as part of the If you or anybody reading would like to review my PR, that will let me merge it so that you can directly build off of it to complete this PR. You may find that in order to complete the docs/tests synchronization, you may need to do one or more of (a) add tests that match the docs; (b) change the docs slightly so that its examples use what we already have in the tests; (c) reorder or slightly change what's in the tests to match the docs or to group things that need to be part of the same docs examples; (d) add new things to one or both or do other kinds of synchronization. Use your best judgment here. But there's no point in the tests-for-tests-sake and tests-for-docs to be rudundant unnecessarily; any parts that can do both simultaneously is better than doing the same thing twice (and making the CI tests more expensive) where one is just for the sake of the docs. |
94346c0 to
c0cb07b
Compare
|
I think what you did here is combine your changes with the commit that contains my recent runtest.py changes. Can you rebase this on the current main and then force-push? What I would really like to see, to make it easiest to review this PR, is for the diff to only contain your changes, and not some combination of yours, mine, and any changes you might have made on top of my changes. |
…atim testsuite test The oiiotool command lines from the 'Channel reordering and padding' section of the oiiotool docs now live verbatim inside run_commands() blocks in the run.py, bracketed by BEGIN/END-docs-channels-* marker comments, and src/doc/oiiotool.md literalincludes each command between its markers -- so the docs and the test are literally the same lines and can no longer drift apart. The setup images use asymmetric per-channel colors on purpose: with the default black/white checker, R and B are identical in every pixel, which made the channel-swap and alpha-from-red examples produce outputs indistinguishable by content hash. Contributes to AcademySoftwareFoundation#3992 Assisted-by: Claude Code / glm-5.3-flash Signed-off-by: linsen <251731047+linsen458-spec@users.noreply.github.com>
c0cb07b to
fb454b6
Compare
|
You're right, sorry — that was my mistake. When rebuilding the branch, I accidentally amended my commit on top of yours, which swallowed your runtest.py changes into the PR diff. I've fixed it now. Thanks for your patience in reviewing and replying. |
|
This looks great. I would actually be fine with merging this as it is now, though I have some questions, and I'm not asking you to necessarily change anything (not yet, anyway), I just am interested in how you think we should proceed next. You made a new test, oiiotool-channels, with oiiotool commands that are examples of But we already did have tests for the So how should we reconcile this? I think this question has two parts:
Am I rambling? |
|
Not rambling at all — if anything, it's good to see how much you care about getting this right. This is exactly the kind of project that makes me happy to spend the time. So I went back through both run.py files and diffed the examples against what oiiotool-copy already has. Two things stood out. On where they live: the two directories are doing different jobs. oiiotool-copy groups by feature, and a lot of its lines lean on files created earlier in the same run.py — src/rgbaz.exr only exists because of the lines above it. oiiotool-channels follows the docs instead, with each example self-contained and the setup commands at the top making whatever it needs. That's mostly why I'd leave the examples where they are. A line lifted from the middle of oiiotool-copy's run.py would leave a reader wondering where the input came from. Moving all the channel tests out is also riskier than it's worth: some of it is edge-case testing (the PNG channel-count warning, for one) that reads oddly as a doc example, and I'd rather not churn a passing test for no clear win. One small directory per section also fits the size and runtime limits you mentioned better than growing the big ones. Then the duplicates. I went through them, and the overlap is smaller than it looks. Of the seven examples, only one repeats what oiiotool-copy already covers ( For that one I'd keep both, honestly. Different inputs, different outputs, and it costs about nothing. If we want a rule going forward, the one you floated earlier works for me: before a docs example becomes a test, check whether an existing test already covers it. Reads fine on its own? Let the docs reuse that line. Depends on earlier setup? The example stays with its section and the old test keeps its job. I'll apply that to the rest of #3992 unless you object. And if you'd rather consolidate by feature and fold these into oiiotool-copy, that's a quick change too — say the word. |
|
If the individual test lines we primarily use for full test coverage just aren't appropriate for what we want in the docs for clear elucidation, then let's keep them both and not worry about a little redundancy. However, if you don't mind, I think I would prefer that these few new tests just land in oiiotool-copy, near the others that also exercise Sometimes when an individual test directory is seen to be very big or expensive, we do try to split it (into roughly equal portions if possible, and hopefully with some obvious logic to which tests go in which directory). In this case, the new tests are few, and they sure seem logically connected to things in oiiotool-copy, so let's not make a new directory for this. Sorry to ask you to do the extra work. I believe this is the last thing to change on this PR. By the way, did you intend this as a DevDays project? |
Part of #3992, reworked on top of the merged #5465.
This reworks the testsuite/oiiotool-channels test so that the oiiotool command lines from the "Channel reordering and padding" section of the docs live verbatim inside run_commands() blocks in the run.py, each bracketed by BEGIN/END-docs-channels-* marker comments. src/doc/oiiotool.md then literalincludes each command between its markers — so the docs and the test are literally the same lines and can no longer drift apart.
The setup images intentionally use asymmetric per-channel colors: with the default black/white checker pattern, R and B are identical in every pixel, which made the channel-swap and alpha-from-red examples produce outputs indistinguishable by content hash.
Testing: the content hashes of all six output images are pinned in out.txt (text-only comparison, no reference images needed), and the test passes locally.
This is not my native language, I'm using an LLM to restate and fix grammar in my replies. All the code was reviewed by me personally, and the description above was reviewed and approved by me as well. Looking forward to contributing more here, and happy to hear any feedback.