diff --git a/test/renderdiff/src/golden_manager.py b/test/renderdiff/src/golden_manager.py index 3075c4505a..a63d83e812 100644 --- a/test/renderdiff/src/golden_manager.py +++ b/test/renderdiff/src/golden_manager.py @@ -16,6 +16,7 @@ import os import shutil import re import sys +import tempfile from utils import execute, ArgParseImpl, mkdir_p @@ -99,10 +100,11 @@ class GoldenManager: code, old_commit = execute(f'git log --format=%B -n 1', cwd=assets_dir) if tag and len(tag) > 0: old_commit += f'\nFILAMENT={tag}' - COMMIT_FILE = '/tmp/golden_commit.txt' - with open(COMMIT_FILE, 'w') as f: + with tempfile.NamedTemporaryFile('w', delete=False) as f: f.write(old_commit) - self._git_exec(f'commit --amend -F {COMMIT_FILE}') + commit_file = f.name + self._git_exec(f'commit --amend -F {commit_file}') + os.remove(commit_file) # Do the actual merge self._git_exec(f'checkout main') @@ -139,11 +141,11 @@ class GoldenManager: os.path.join(rdiff_dir, f)) self._git_exec(f'add {os.path.join(GOLDENS_DIR, f)}') - TMP_GOLDEN_COMMIT_FILE = '/tmp/golden_commit.txt' - - with open(TMP_GOLDEN_COMMIT_FILE, 'w') as f: + with tempfile.NamedTemporaryFile('w', delete=False) as f: f.write(commit_msg) - self._git_exec(f'commit -a -F {TMP_GOLDEN_COMMIT_FILE}') + tmp_golden_commit_file = f.name + self._git_exec(f'commit -a -F {tmp_golden_commit_file}') + os.remove(tmp_golden_commit_file) if push_to_remote and \ (self.access_token_ or self.access_type_ == ACCESS_TYPE_SSH): self._git_exec(f'push -f origin {branch}') diff --git a/test/renderdiff/src/update_golden.py b/test/renderdiff/src/update_golden.py index 3ea600930e..77ad055b28 100644 --- a/test/renderdiff/src/update_golden.py +++ b/test/renderdiff/src/update_golden.py @@ -64,19 +64,19 @@ def _do_update(golden_manager, config): def _same_image_diffimg(diffimg_path, img1, img2): cmd = [diffimg_path, img1, img2] - try: - result = subprocess.run(cmd, capture_output=True, text=True) - output = result.stdout.strip() - if not output: - return False + result = subprocess.run(cmd, capture_output=True, text=True) + if result.returncode != 0: + raise RuntimeError(f"diffimg failed with return code {result.returncode}:\n{result.stderr}") - try: - res_json = json.loads(output) - return res_json.get('passed', False) - except json.JSONDecodeError: - return False - except Exception: - return False + output = result.stdout.strip() + if not output: + raise RuntimeError("diffimg produced no output") + + try: + res_json = json.loads(output) + return res_json.get('passed', False) + except json.JSONDecodeError as e: + raise RuntimeError(f"Failed to parse diffimg output: {e}\nOutput: {output}") def _get_deletes_updates(update_dir, golden_dir, diffimg_path): ret_delete = [] @@ -93,13 +93,13 @@ def _get_deletes_updates(update_dir, golden_dir, diffimg_path): # However, update_golden typically only updates/adds based on the new render set. # But strict sync might imply deleting missing ones. # The original logic was: delete = list(base - new). - delete = list(base - new) + delete_files = list(base_files - new_files) # Files in new but not in base are definitely updates (additions) - update = list(new - base) + update_files = list(new_files - base_files) # Files in both need comparison - for fpath in base.intersection(new_files): + for fpath in base_files.intersection(new_files): base_fpath = os.path.join(golden_dir, fpath) new_fpath = os.path.join(update_dir, fpath) @@ -110,10 +110,10 @@ def _get_deletes_updates(update_dir, golden_dir, diffimg_path): is_different = _file_as_str(new_fpath) != _file_as_str(base_fpath) if is_different: - update.append(fpath) + ret_update.append(fpath) - ret_update += update - ret_delete += delete + ret_update += update_files + ret_delete += delete_files return ret_delete, ret_update @@ -179,7 +179,7 @@ if __name__ == "__main__": parser = ArgParseImpl() parser.add_argument('--branch', help='Branch of the golden repo to write to') parser.add_argument('--golden-repo-token', help='Access token for the golden repo') - parser.add_argument('--push-to-remote', action="store_true", help='Access token for the golden repo') + parser.add_argument('--push-to-remote', action="store_true", help='Push the golden repo changes to remote') parser.add_argument('--diffimg', help='Path to the diffimg tool', default='./out/cmake-release/tools/diffimg/diffimg') diff --git a/test/renderdiff/src/utils.py b/test/renderdiff/src/utils.py index fdac66e3e8..34c92d5586 100644 --- a/test/renderdiff/src/utils.py +++ b/test/renderdiff/src/utils.py @@ -18,6 +18,7 @@ import shutil import argparse import sys import pathlib +import shlex def execute(cmd, cwd=None, @@ -25,7 +26,7 @@ def execute(cmd, stdin=None, env=None, raise_errors=False): - in_env = os.environ + in_env = os.environ.copy() in_env.update(env if env else {}) home = os.environ['HOME'] if f'{home}/bin' not in in_env['PATH']: @@ -45,11 +46,11 @@ def execute(cmd, 'universal_newlines': True } if capture_output: - process = subprocess.Popen(cmd.split(' '), **kwargs) + process = subprocess.Popen(shlex.split(cmd), **kwargs) output, err_output = process.communicate() return_code = process.returncode else: - return_code = subprocess.call(cmd.split(' '), **kwargs) + return_code = subprocess.call(shlex.split(cmd), **kwargs) if return_code: # Error