Refactor test output paths and make CliOptions immutable#32
Draft
ppenenko wants to merge 2 commits into
Draft
Conversation
When output_dir and shader_baseline_dir resolve to the same directory, shutil.copy2 fails on Windows (PermissionError copying a file onto itself). The copy was unnecessary: rendertest already dumps shaders directly into the baseline directory during local development. Simplify the function to only run the _RefDiffer comparison in CI mode (when render_output_dir is set). Signed-off-by: Pavlo Penenko <pavlo.penenko@autodesk.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the shader-baseline handling in the render test suite by removing the “update mode” baseline-copy path from _handle_shader_baselines, leaving only baseline comparison behavior intended for CI runs.
Changes:
- Removed the
shutil.copy2update-mode branch from_handle_shader_baselines. - Tightened the guard so shader baseline handling only runs when
render_output_diris set (CI mode). - Updated the function docstring to reflect CI-only comparison intent.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+456
to
+460
| """Compare dumped shaders against committed baselines in CI mode. | ||
|
|
||
| * **Update mode** (no ``render_output_dir``): copy dumped shaders into | ||
| the committed baseline directory for ``git diff`` review. | ||
| * **CI mode** (``render_output_dir`` set): compare dumped shaders | ||
| against the committed baselines and assert on mismatch. | ||
| Requires both ``shader_baseline_dir`` and ``render_output_dir`` to be | ||
| set. Shaders are dumped directly into the baseline directory during | ||
| local development (no copy step needed). |
Comment on lines
+462
to
+469
| if (not result.shader_dump_paths or not opts.shader_baseline_dir | ||
| or not opts.render_output_dir): | ||
| return | ||
| baseline_subdir = opts.shader_baseline_dir / stem | ||
| if opts.render_output_dir: | ||
| differ = _RefDiffer(baseline_subdir) | ||
| for dump_path in result.shader_dump_paths.values(): | ||
| if (baseline_subdir / dump_path.name).exists(): | ||
| differ(dump_path) | ||
| else: | ||
| import shutil | ||
| baseline_subdir.mkdir(parents=True, exist_ok=True) | ||
| for dump_path in result.shader_dump_paths.values(): | ||
| shutil.copy2(dump_path, baseline_subdir / dump_path.name) | ||
| differ = _RefDiffer(baseline_subdir) | ||
| for dump_path in result.shader_dump_paths.values(): | ||
| if (baseline_subdir / dump_path.name).exists(): | ||
| differ(dump_path) |
- Make CliOptions frozen; move output_dir and flat_layout to RenderEnvironment so each environment owns its paths. - Default --output-dir to contrib/ (the common root); environments append renders/, renders/adsk/, tests/metashade_ref/renders/<sub>. - Remove --render-output-dir, shader_baseline_dir, _RefDiffer, and _handle_shader_baselines (all now redundant). - Collapse OVERRIDE_SUBDIR / OUTPUT_SUBDIR into single SUBDIR field. - Rename self.options → self.cli_options for clarity. Signed-off-by: Pavlo Penenko <pavlo.penenko@autodesk.com>
ppenenko
changed the title
Comment on lines
+25
to
28
| """Paths for Metashade reference data, relative to ``--output-dir``.""" | ||
| ROOT = Path("tests") / "metashade_ref" | ||
| LIBRARIES = ROOT / "libraries" | ||
| RENDERS = ROOT / "renders" |
| ) | ||
|
|
||
| libraries_dir = repo_root / _RefPaths.LIBRARIES | ||
| libraries_dir = cli_options.output_dir / _RefPaths.LIBRARIES |
Comment on lines
+139
to
+140
| output_dir = cli_options.output_dir / _RefPaths.RENDERS / subdir | ||
| output_dir.mkdir(parents=True, exist_ok=True) |
Comment on lines
+45
to
+46
| Per-environment settings (``output_dir``, ``flat_layout``) live on | ||
| :class:`RenderEnvironment`. |
ppenenko
marked this pull request as draft
July 17, 2026 14:38
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
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
CliOptionsfrozen (immutable); moveoutput_dirandflat_layouttoRenderEnvironmentso each environment owns its paths.--output-dirtocontrib/(the common root); environments appendrenders/,renders/adsk/,tests/metashade_ref/renders/<sub>.--render-output-dir,shader_baseline_dir,_RefDiffer, and_handle_shader_baselines— all now redundant.OVERRIDE_SUBDIR/OUTPUT_SUBDIRinto a singleSUBDIRfield.self.optionstoself.cli_optionsfor clarity.Motivation
The previous output-path plumbing grew organically and accumulated redundant fields (
shader_baseline_dir,render_output_dir) and a copy step (_handle_shader_baselines/_RefDiffer) that was a no-op in local dev and caused aPermissionErroron Windows when source and destination were the same file.This refactor makes the path hierarchy explicit:
cli_options.output_dir— CLI root (defaultcontrib/)renders/,renders/adsk/,tests/metashade_ref/renders/<subdir>get_output_path()Test plan
test_render_metashade.py)test_render.pysuite passes (stdlib + adsk environments)