Skip to content

Get rid of unique ids in SVG files. #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 16, 2024
Merged

Get rid of unique ids in SVG files. #14

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions pytest_textual_snapshot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from operator import attrgetter
from os import PathLike
from pathlib import Path, PurePath
from random import random
from tempfile import mkdtemp
from typing import Any, Awaitable, Union, Optional, Callable, Iterable, TYPE_CHECKING

Expand All @@ -33,6 +34,18 @@ class SVGImageExtension(SingleFileSnapshotExtension):
_file_extension = "svg"
_write_mode = WriteMode.TEXT

def _read_snapshot_data_from_location(self, *args, **kwargs) -> Optional["SerializableData"]:
"""Normalize SVG data right after they are loaded from persistent storage."""
data = super()._read_snapshot_data_from_location(*args, **kwargs)
if data is not None:
data = normalize_svg(data)
return data
Comment on lines +37 to +42
Copy link
Member

@darrenburns darrenburns Dec 3, 2024

Choose a reason for hiding this comment

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

I'm not sure about this. Subclassing a private method from a non-pinned 3rd party library is likely to eventually break if the method is removed or renamed.

Copy link
Contributor Author

@xavierog xavierog Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, but I simply obeyed this:
https://github.com/syrupy-project/syrupy/blob/ef8189c68460593fead9c484405b755e272c8cca/src/syrupy/extensions/base.py#L140C10-L140C43

SnapshotCollectionStorage.read_snapshot(): This method is final, do not override. You can override _read_snapshot_data_from_location in a subclass to change behaviour.

Copy link
Contributor Author

@xavierog xavierog Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@darrenburns So, what do you think?
By the way, we could pin syrupy as an extra security measure.

Copy link
Member

@darrenburns darrenburns Dec 12, 2024

Choose a reason for hiding this comment

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

Sorry!

I think it makes sense to pin syrupy and merge this. Although perhaps longer term it'd make sense to have an option in Textual to write SVGs without the IDs rather than stripping them out.

Copy link
Contributor Author

@xavierog xavierog Dec 13, 2024

Choose a reason for hiding this comment

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

Sorry!

No problem, the week before the 1.0.0 release must have been quite busy.

I think it makes sense to pin syrupy and merge this.

Although perhaps longer term it'd make sense to have an option in Textual to write SVGs without the IDs rather than stripping them out.

Stricto sensu, that option already exists... but it cannot be leveraged from pytest-textual-snapshot.

As part of my personal reflections following this PR, I think the ideal situation is to introduce a specialized, diff-friendly format that represents a TUI screenshot and can be turned into an SVG picture. That new format would be versioned into Git instead of the SVG pictures whereas the SVG pictures would still appear in the HTML report. I started imagining a simple text format based on layers (background colors, text, foreground colors) but quickly noticed it lacks an elegant way to embed text formatting like bold, underline, etc. -- should this become a fourth layer or should it be shoehorned into the other layers? In the end, this endeavour is a significant amount of work for a rather small improvement, at a time when Textual users expect improvements in other areas (e.g. more widgets). So... yeah, definitely a longer term idea.


def serialize(self, *args, **kwargs) -> "SerializedData":
"""Normalize SVG data before they get compared against a snapshot and
before they get persisted to storage."""
return normalize_svg(super().serialize(*args, **kwargs))


class TemporaryDirectory:
"""A temporary that survives forking.
Expand Down Expand Up @@ -71,10 +84,14 @@ class PseudoApp:
console: PseudoConsole


def rename_styles(svg: str, suffix: str) -> str:
"""Rename style names to prevent clashes when combined in HTML report."""
return re.sub(r"terminal-(\d+)-r(\d+)", rf"terminal-\1-r\2-{suffix}", svg)
def individualize_svg(svg: str, unique_id: Optional[str] = None) -> str:
"""Inject a random id, à la rich.Console.export_svg()."""
unique_id = str(int(random() * 1e10)) if unique_id is None else unique_id
return re.sub(r"\bterminal(?:-\d+)?-([\w-]+)", rf"terminal-{unique_id}-\1", svg)

def normalize_svg(svg: str) -> str:
"""Strip the unique id generated by rich.Console.export_svg()."""
return re.sub(r"\bterminal-\d+-([\w-]+)", r"terminal-\1", svg)

def pytest_addoption(parser):
parser.addoption(
Expand Down Expand Up @@ -301,8 +318,8 @@ def retrieve_svg_diffs(
n += 1
diffs.append(
SvgSnapshotDiff(
snapshot=rename_styles(str(expect_svg_text), f"exp{n}"),
actual=rename_styles(svg_text, f"act{n}"),
snapshot=individualize_svg(str(expect_svg_text)),
actual=individualize_svg(svg_text),
test_name=name,
path=full_path,
line_number=line_index + 1,
Expand Down