Skip to content
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.

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

Kompari xtask (html report, interactive test blessing, dead snapshots detection) #272

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

spirali
Copy link
Contributor

@spirali spirali commented Feb 12, 2025

Adds command cargo xtask that allows to:

Creating report

cargo xtask report

creates HTML file with report containing snapshots that does not match

Interactive test blessing:

cargo xtask review

stars a local web server with the following page:

(screenshot obtained by putting an artificial error into each snapshot test)
Screenshot 2025-02-12 at 13-15-47 Kompari review

Detecting dead snapshots

cargo xtask dead-snapshots

This PR fully replaces #185

Copy link
Member

@tomcur tomcur left a comment

Choose a reason for hiding this comment

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

Very cool! I made some tests intentionally fail subtly, and I think this will definitely help with e.g. spotting when there are actual issues vs just rounding differences.

Some minor nits inline, and I think you intended to commit .cargo/config.toml or something similar, I added a comment about that on parley/tests/README.md.

@@ -20,15 +20,15 @@ checksum = "c71b1793ee61086797f5c80b6efa2b8ffa6d5dd703f118545808a7f2e27f7046"

[[package]]
name = "accesskit"
version = "0.17.0"
version = "0.17.1"
Copy link
Member

Choose a reason for hiding this comment

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

I believe according to https://github.com/linebender/rfcs/blob/main/rfcs/0005-version-matrix.md#cargotoml-matches-cargolock, Cargo.toml should be updated to match these patch versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be fixed now by running cargo upgrade

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, for example Cargo.toml still lists accesskit = "0.17", but it's locked to "0.17.1" in the lockfile.

Maybe @DJMcNab can chime in?

Copy link
Member

Choose a reason for hiding this comment

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

We should always specify the patch version in Cargo.toml, yes. So we never should have a dependency of the form accesskit = "0.17".

That's largely a thing for consistency, but it does have advantages for e.g. when we publish, largely in case any user decides to use the MSRV-aware resolver or minimal-versions.

Comment on lines +35 to +40
let args = Args::parse();
let mut diff_config = DirDiffConfig::new(snapshots_path, current_path);
diff_config.set_ignore_right_missing(true);
let actions = ActionsImpl();
let mut task = Task::new(diff_config, Box::new(actions));
task.run(&args)?;
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to move more of this into kompari_tasks? If more repositories use Kompari, they'd all duplicate this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the original plan, but I still want to have a relatively high level of configurability, as it seems that the Vello testing framework works a bit differently than what I implemented for Parley.

@@ -20,15 +20,15 @@ checksum = "c71b1793ee61086797f5c80b6efa2b8ffa6d5dd703f118545808a7f2e27f7046"

[[package]]
name = "accesskit"
version = "0.17.0"
version = "0.17.1"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, for example Cargo.toml still lists accesskit = "0.17", but it's locked to "0.17.1" in the lockfile.

Maybe @DJMcNab can chime in?

@spirali
Copy link
Contributor Author

spirali commented Mar 12, 2025

Kompari updated. It adds cargo xtask size-check that checks optimization opportunities for snapshots.

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.

3 participants