Summary: | layout test checksums should be embedded in the png as a comment | ||
---|---|---|---|
Product: | WebKit | Reporter: | Tony Chang <tony> |
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | dglazkov, dpranke, eric, jparent, mihaip, ojan, ossy |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | PC | ||
OS: | OS X 10.5 | ||
Bug Depends on: | 57255, 57280, 57481, 57575, 57783, 57871, 57993, 58021, 58173, 58387, 58402 | ||
Bug Blocks: |
Description
Tony Chang
2011-03-13 20:58:20 PDT
(In reply to comment #0) > - harder to view/manually edit the checksum, e.g. it won't be in the diff. I'm not sure anyone ever actually does this. I've used this to quickly determine whether chromium-mac baselines should be moved to mac-leopard (i.e. if a test fails on Mac Leopard, I've run it with --reset-results and checked whether the new checksum was the same as the one that was in the Chromium directory). However, I think I could use ImageDiff for this too. (In reply to comment #1) > (In reply to comment #0) > > - harder to view/manually edit the checksum, e.g. it won't be in the diff. I'm not sure anyone ever actually does this. > > I've used this to quickly determine whether chromium-mac baselines should be moved to mac-leopard (i.e. if a test fails on Mac Leopard, I've run it with --reset-results and checked whether the new checksum was the same as the one that was in the Chromium directory). However, I think I could use ImageDiff for this too. I could also write some tools to help with this. E.g., it seems like PrettyPatch.rb could know how to pull the checksum out of the png or another script. It might be helpful to sketch out the longer-term plan for how we get rid of the .checksum files, e.g., what lands when, how do we update the rebaselining tools, will other ports be able to support this, how many checksum files are generic vs. port-specific, etc. It seems like a good idea in the end-state but if only some PNGs will ever have checksums in them, it seems like this might make things more confusing rather than less, by virtue of the two code paths. I agree with Dirk. I've occasionally "promoted" chromium-mac baselines to mac (or mac-leopard) as part of getting the Chromium Snow Leopard bot green, it'll be more difficult to do this if the Mac port expects .checksum files (though this could be mitigated with tools to re-generate the standalone .checksum file from a .png). Sure, I plan on sending an email to the gardeners mailing list once the rebaseline tool supports this (the patch has been reviewed but not yet landed). Until then, this change and the change in bug 57280 doesn't actually impact current behavior so they should be harmless to land. (In reply to comment #5) > Until then, this change and the change in bug 57280 doesn't actually impact current behavior so they should be harmless to land. Err, the change in bug 57255 and bug 57280. I like the idea. Certainly, if we can get all ports to do it this way it'd be better. My concern is what we do in the interim where Chromium does this. I guess we check for the checksum first, so it just works? Also, it would be nice for the layout test dashboard to be able to show the checksums for easily comparing the images from different ports. Although, I guess it would be better if the dashboard just let you diff any two images. As for your pros/cons, I don't see any benefit in people manually editing checksums. (In reply to comment #7) > My concern is what we do in the interim where Chromium does this. I guess we check for the checksum first, so it just works? That's correct. We first check to see if there's a checksum and only if one doesn't exist do we look at the png comment. > Also, it would be nice for the layout test dashboard to be able to show the checksums for easily comparing the images from different ports. Although, I guess it would be better if the dashboard just let you diff any two images. This should be possible to do in JS via XHR. I think we need to add "Access-Control-Allow-Origin: *" to build.chromium.org/f/chromium/layout_test_results/ so we can fetch the resource, but otherwise this seems possible. I'll talk to the bot admins to see what needs to be done to allow this. I declare victory on this bug. There's still some obsolete code in NRWT that needs to be deleted and some .checksum files will probably creep in over the next few days, but that's cleanup I'll address in the next few work days. |