-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
Conversation
There was a problem hiding this 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.
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..." |
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.) |
There was a problem hiding this 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.
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.
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."
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. |
Thanks for the explanation! |
No description provided.