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

test: add a way to regenerate test data #127

Closed
wants to merge 1 commit into from

Conversation

sorawee
Copy link

@sorawee sorawee commented Jan 27, 2024

No description provided.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Resyntax analyzed 0 files in this pull request and found no issues.

@alex-hhh
Copy link
Collaborator

Not sure what you aim with this PR, but I wanted it to make it more difficult to quickly override images and draw steps for failed tests.

Currently, the developer has to stop and think and manually verify that each of the failing tests don't introduce actual visual problems with the plot. With your proposed change it becomes too easy for a developer to simply decide "I'm sure my changes are OK, let me just update all these tests at once and go..."

@sorawee
Copy link
Author

sorawee commented Jan 27, 2024

My two cents:

They won't be able to "quickly override images and draw steps for failed tests", because there is a review process.

On the other hand, overriding the file allows one to compare images in GitHub diff view, which is much easier than finding two specific files from a directory with hundred files, and then arrange windows to compare them side by side.

To me, this is simply a convenience tool.

I trust your judgment though. If you don't think it makes sense, feel free to close it.

(Also, it should be merged after #128, if it's to be merged.)

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Resyntax analyzed 0 files in this pull request and found no issues.

@alex-hhh
Copy link
Collaborator

I considered adding this feature when I originally implemented these tests but decided not to.

Making it too convenient to update these tests introduces the risk of putting backwards incompatible changes in the plot package, where the output of the plot changes between releases (even if the API stays the same).

The current setup is not ideal, but it does not seem to prevent or hinder people from contributing features and fixes to the plot package (many contributions were done since I introduced this system several years ago). If this becomes a practical problem in the future, I'll reconsider it.


They won't be able to "quickly override images and draw steps for failed tests", because there is a review process.

The review process is not perfect and can fail too. In my professional field, the systems we design have multi-layered safety features and one cannot justify removing safety features just because "there is at least one safety feature remaining."

On the other hand, overriding the file allows one to compare images in GitHub diff view, which is much easier than finding two specific files from a directory with hundred files, and then arrange windows to compare them side by side.

Comparing images is easy in Github, but as you noted yourself in #126 , images will never be identical, and it is easy to look at images, and decide that any changes on the image are caused by platform specific differences.

The ".DAT" files, which contain the rendering steps, are the true indicator of what changed, and if a change is backwards compatible or not. Unfortunately, these binary files cannot be diff-ed in Github, since they are compressed, to save in file size.

Making it "convenient" would make people rely on the Github image diff, which is the wrong thing to do.

@sorawee
Copy link
Author

sorawee commented Jan 28, 2024

Thanks for the explanation!

@sorawee sorawee closed this Jan 28, 2024
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.

2 participants