Skip to content

Handle ImageMagick 6 and 7 command names in tests#68

Closed
iwillspeak wants to merge 1 commit into
mainfrom
fix/more-magick
Closed

Handle ImageMagick 6 and 7 command names in tests#68
iwillspeak wants to merge 1 commit into
mainfrom
fix/more-magick

Conversation

@iwillspeak

Copy link
Copy Markdown
Contributor

No description provided.

@iwillspeak iwillspeak enabled auto-merge (squash) December 2, 2025 17:09
@iwillspeak iwillspeak requested review from anna-singleton-resolver, Copilot and snus-kin and removed request for Copilot and snus-kin December 2, 2025 17:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the ImageMagick command detection in functional tests to support both ImageMagick version 7.x (using the magick command) and version 6.x (using the convert command). This ensures the tests can run successfully regardless of which ImageMagick version is installed.

Key changes:

  • Added fallback logic to check for the convert command when magick is not found
  • Updated the error message to be version-agnostic

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/functional/conftest.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +96 to +98
if (magick_path := shutil.which("magick")) is None and (
magick_path := shutil.which("convert")
) is None:

Copilot AI Dec 2, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convert command is ambiguous as it exists in both ImageMagick and other system utilities. Consider using magick convert for ImageMagick 7.x compatibility or checking for convert from ImageMagick specifically by verifying its version output to ensure the correct tool is being used.

Copilot uses AI. Check for mistakes.
Comment on lines 124 to 133
cmd = f'magick "{base_image_path}" "{image_path}"'
_ = subprocess.run( # noqa: S603 - false positive :(
[magick_path, str(base_image_path), str(image_path)],
check=True,
shell=False,
)

if not image_path.exists():
pytest.fail(
f"Failed to create {image_format} image with command: {cmd}"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cmd = [magick_path, str(base_image_path), str(image_path)]
_ = subprocess.run( # noqa: S603 - false positive :(
cmd,
check=True,
shell=False,
)
if not image_path.exists():
pytest.fail(
f"Failed to create {image_format} image with command: {cmd}"

not gonna block over it but that error isnt quite right anymore, and setting cmd and then not using it is confusing

@iwillspeak iwillspeak disabled auto-merge December 15, 2025 11:19
@iwillspeak iwillspeak closed this Dec 15, 2025
@iwillspeak iwillspeak deleted the fix/more-magick branch February 12, 2026 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants