88456 2012-06-06 14:38:22 -0700 get rebaselining tools to kinda work with the skia overrides file 2012-06-07 09:08:47 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 dpranke dpranke abarth dglazkov epoger ojan rniwa oldest_to_newest 642822 0 dpranke 2012-06-06 14:38:22 -0700 We recently added support for a "skia" overrides file for the chromium port, so that the skia devs can more easily manage skia changes that require a lot of rebaselining (bug 86749). Unfortunately, that broke how rebaselining worked for the main files (see bug 87406). The root of the problem is that the test expectations code does not maintain a mapping of actual expectation entries to the text files they are found in. So, when we need to write out an updated file after removing a REBASELINE entry, we serialize all of the entries (across all of the files) into a single text file. This was bad because it would take the contents of the skia file and insert them into the main expectations file :(. So, in bug 87406 we disabled looking at the overrides files altogether during rebaselining, which fixes the normal/gardener use case for rebaselining, but breaks the skia dev use case. One correct long term fix would be to maintain the entry<->file mapping for all of the entries; we would need this in order to support cascaded expectations files. Of course, another approach would be to stop using the REBASELINE keyword in the file, but we'll probably need the mapping to make cascading maintainable/debuggable regardless. Cascading expectations are being tracked in bug 65834. In the meantime, in order to unbreak the skia devs, I'm going to add a switch that will optionally restore the "broken" behavior we had in 86749; this seems to be an awkward but acceptable workflow for the skia devs and is a minimal amount of work, allowing me to focus on fixing 64834. 642823 1 dpranke 2012-06-06 14:38:40 -0700 er, that's 65834, not 64834. 642851 2 rniwa 2012-06-06 15:00:09 -0700 (In reply to comment #0) > In the meantime, in order to unbreak the skia devs, I'm going to add a switch that will optionally restore the "broken" behavior we had in 86749; this seems to be an awkward but acceptable workflow for the skia devs and is a minimal amount of work, allowing me to focus on fixing 64834. I don't see how adding the broken behavior back using a command line switch will help. How does copying skia_test_expectations.txt entries into TestExpectations for every rebaseline we do ever help? Skia folks can already rebaseline tests as expected because tests marked failing in skia_test_expectations.txt still show up on garden-o-matic's expected failures' tab, and you can still rebaseline those tests successfully only updating entries in test_expectations.txt. We had never supported skia_test_expectations.txt so introducing the old behavior isn't going to help skia folks at all. 642897 3 dpranke 2012-06-06 15:36:35 -0700 (In reply to comment #2) > (In reply to comment #0) > > In the meantime, in order to unbreak the skia devs, I'm going to add a switch that will optionally restore the "broken" behavior we had in 86749; this seems to be an awkward but acceptable workflow for the skia devs and is a minimal amount of work, allowing me to focus on fixing 64834. > > I don't see how adding the broken behavior back using a command line switch will help. How does copying skia_test_expectations.txt entries into TestExpectations for every rebaseline we do ever help? Skia folks can already rebaseline tests as expected because tests marked failing in skia_test_expectations.txt still show up on garden-o-matic's expected failures' tab, and you can still rebaseline those tests successfully only updating entries in test_expectations.txt. > > We had never supported skia_test_expectations.txt so introducing the old behavior isn't going to help skia folks at all. As discussed over IRC, the point is to ensure that if the skia devs put "REBASELINE" into skia_test_expectations.txt, we'll actually rebaseline the test. They don't care about garbage getting written into TestExpectations, because they won't commit that change, and they're okay with skia_test_expectations.txt not getting updated properly for now. Also, you correctly noted that we don't actually need a flag to get the right behavior. in the rebaseline-expectations command, as long as we include the overrides when getting the list of files to rebaseline, but don't include them when writing the expectations back out, we should be okay. 642902 4 146130 dpranke 2012-06-06 15:38:18 -0700 Created attachment 146130 Patch 642905 5 146130 rniwa 2012-06-06 15:40:48 -0700 Comment on attachment 146130 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146130&action=review > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:264 > + def _expectations(self, port, include_overrides): > + return TestExpectations(port, include_overrides=include_overrides) Do we really need this method still? 642908 6 dpranke 2012-06-06 15:44:59 -0700 Committed r119628: <http://trac.webkit.org/changeset/119628> 642910 7 dpranke 2012-06-06 15:45:33 -0700 (In reply to comment #5) > Do we really need this method still? Probably not :). 643599 8 ojan 2012-06-07 09:08:47 -0700 (In reply to comment #0) > The root of the problem is that the test expectations code does not maintain a mapping of actual expectation entries to the text files they are found in. So, when we need to write out an updated file after removing a REBASELINE entry, we serialize all of the entries (across all of the files) into a single text file. This was bad because it would take the contents of the skia file and insert them into the main expectations file :(. Rebaseline entry aside, we want to have automated tooling like garden-o-matic be able to remove entries from the right file when we have multiple cascading files in the tree. 146130 2012-06-06 15:38:18 -0700 2012-06-06 15:40:47 -0700 Patch bug-88456-20120606153817.patch text/plain 5054 dpranke U3VidmVyc2lvbiBSZXZpc2lvbjogMTE5NjE2CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggYzliMjViY2U4Yzc4OGVlNTM4YTE1NjZhYjRjNmQ0M2I1 ZGVkNWM1Ny4uMGNlYTdlZjkzZTU5MWQwYTE1NTM1NzcyZGRhZmI0MjcyMjY1NTY1YyAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDI2 IEBACiAyMDEyLTA2LTA2ICBEaXJrIFByYW5rZSAgPGRwcmFua2VAY2hyb21pdW0ub3JnPgogCisg ICAgICAgIGdldCByZWJhc2VsaW5pbmcgdG9vbHMgdG8ga2luZGEgd29yayB3aXRoIHRoZSBza2lh IG92ZXJyaWRlcyBmaWxlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD04ODQ1NgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgIFVwZGF0ZSByZWJhc2VsaW5lLWV4cGVjdGF0aW9ucyBzbyB0aGF0IHdlIHJlYWQgaW4g dGhlIG92ZXJyaWRlcworICAgICAgICB3aGVuIGxvb2tpbmcgZm9yIHRlc3RzIHRvIHJlYmFzZWxp bmUsIGJ1dCBkb24ndCByZWFkIGluIHRoZQorICAgICAgICBvdmVycmlkZXMgd2hlbiB3ZSBhcmUg d3JpdGluZyB0aGUgZXhwZWN0YXRpb25zIGZpbGUgYmFjayBvdXQuCisgICAgICAgIFRoaXMgcHJl dmVudHMgdGhlIG92ZXJyaWRlcyBmcm9tIGdldHRpbmcgd3JpdHRlbiBpbnRvIHRoZSBtYWluCisg ICAgICAgIGZpbGUuIFRoaXMgaXMga2luZCBvZiBhIGhhY2sgYnV0IHdpbGwgaGF2ZSB0byBkbyB1 bnRpbCB3ZQorICAgICAgICBzdXBwb3J0IG11bHRpcGxlIGV4cGVjdGF0aW9ucyBmaWxlcyBwcm9w ZXJseS4KKworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvdG9vbC9jb21tYW5kcy9yZWJhc2Vs aW5lLnB5OgorICAgICAgICAoUmViYXNlbGluZUV4cGVjdGF0aW9ucy5fZXhwZWN0YXRpb25zKToK KyAgICAgICAgKFJlYmFzZWxpbmVFeHBlY3RhdGlvbnMuX3VwZGF0ZV9leHBlY3RhdGlvbnNfZmls ZSk6CisgICAgICAgIChSZWJhc2VsaW5lRXhwZWN0YXRpb25zLl90ZXN0c190b19yZWJhc2VsaW5l KToKKyAgICAgICAgKiBTY3JpcHRzL3dlYmtpdHB5L3Rvb2wvY29tbWFuZHMvcmViYXNlbGluZV91 bml0dGVzdC5weToKKyAgICAgICAgKHRlc3Rfb3ZlcnJpZGVzX2FyZV9pbmNsdWRlZF9jb3JyZWN0 bHkpOgorCisyMDEyLTA2LTA2ICBEaXJrIFByYW5rZSAgPGRwcmFua2VAY2hyb21pdW0ub3JnPgor CiAgICAgICAgIHdlYmtpdHB5OiB0d28gbWFuYWdlcl93b3JrZXJfYnJva2VyX3VuaXR0ZXN0IHRl c3RzIGFyZSBicm9rZW4KICAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTg4NDQ1CiAKZGlmZiAtLWdpdCBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvdG9vbC9j b21tYW5kcy9yZWJhc2VsaW5lLnB5IGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS90b29sL2NvbW1h bmRzL3JlYmFzZWxpbmUucHkKaW5kZXggODA1MTg4ZTY1M2YyZDk2Y2E4ZTg0OTU0ZTU0MDAwNWIz ZGJmOTAxMS4uN2UxYWQ5YzA0YTliYjY0MmJjOTJkYjdlNDU2MDlhMmJiNTljMmJhZiAxMDA2NDQK LS0tIGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS90b29sL2NvbW1hbmRzL3JlYmFzZWxpbmUucHkK KysrIGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS90b29sL2NvbW1hbmRzL3JlYmFzZWxpbmUucHkK QEAgLTI2MCwxOSArMjYwLDI0IEBAIGNsYXNzIFJlYmFzZWxpbmVFeHBlY3RhdGlvbnMoQWJzdHJh Y3REZWNsYXJhdGl2ZUNvbW1hbmQpOgogICAgICAgICAjIEZJWE1FOiBTdXBwb3J0IG5vbi1DaHJv bWl1bSBwb3J0cy4KICAgICAgICAgcmV0dXJuIHBvcnRfbmFtZS5zdGFydHN3aXRoKCdjaHJvbWl1 bS0nKQogCi0gICAgZGVmIF9leHBlY3RhdGlvbnMoc2VsZiwgcG9ydCk6Ci0gICAgICAgIHJldHVy biBUZXN0RXhwZWN0YXRpb25zKHBvcnQpCisgICAgZGVmIF9leHBlY3RhdGlvbnMoc2VsZiwgcG9y dCwgaW5jbHVkZV9vdmVycmlkZXMpOgorICAgICAgICByZXR1cm4gVGVzdEV4cGVjdGF0aW9ucyhw b3J0LCBpbmNsdWRlX292ZXJyaWRlcz1pbmNsdWRlX292ZXJyaWRlcykKIAogICAgIGRlZiBfdXBk YXRlX2V4cGVjdGF0aW9uc19maWxlKHNlbGYsIHBvcnRfbmFtZSk6CiAgICAgICAgIGlmIG5vdCBz ZWxmLl9pc19zdXBwb3J0ZWRfcG9ydChwb3J0X25hbWUpOgogICAgICAgICAgICAgcmV0dXJuCiAg ICAgICAgIHBvcnQgPSBzZWxmLl90b29sLnBvcnRfZmFjdG9yeS5nZXQocG9ydF9uYW1lKQotICAg ICAgICBleHBlY3RhdGlvbnMgPSBzZWxmLl9leHBlY3RhdGlvbnMocG9ydCkKKworICAgICAgICAj IEZJWE1FOiBUaGlzIHdpbGwgaW50ZW50aW9uYWxseSBza2lwIG92ZXIgYW55IFJFQkFTRUxJTkUg ZXhwZWN0YXRpb25zIHRoYXQgd2VyZSBpbiBhbiBvdmVycmlkZXMgZmlsZS4KKyAgICAgICAgIyBU aGlzIGlzIG5vdCBnb29kLCBidXQgYXZvaWRzIGhhdmluZyB0aGUgb3ZlcnJpZGVzIGdldHRpbmcg d3JpdHRlbiBpbnRvIHRoZSBtYWluIGZpbGUuCisgICAgICAgICMgU2VlIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04ODQ1NiBmb3IgY29udGV4dC4gVGhpcyB3aWxsIG5v IGxvbmdlciBiZSBuZWVkZWQKKyAgICAgICAgIyBvbmNlIHdlIHByb3Blcmx5IHN1cHBvcnQgY2Fz Y2FkaW5nIGV4cGVjdGF0aW9ucyBmaWxlcy4KKyAgICAgICAgZXhwZWN0YXRpb25zID0gc2VsZi5f ZXhwZWN0YXRpb25zKHBvcnQsIGluY2x1ZGVfb3ZlcnJpZGVzPUZhbHNlKQogICAgICAgICBwYXRo ID0gcG9ydC5wYXRoX3RvX3Rlc3RfZXhwZWN0YXRpb25zX2ZpbGUoKQogICAgICAgICBzZWxmLl90 b29sLmZpbGVzeXN0ZW0ud3JpdGVfdGV4dF9maWxlKHBhdGgsIGV4cGVjdGF0aW9ucy5yZW1vdmVf cmViYXNlbGluZWRfdGVzdHMoZXhwZWN0YXRpb25zLmdldF9yZWJhc2VsaW5pbmdfZmFpbHVyZXMo KSkpCiAKICAgICBkZWYgX3Rlc3RzX3RvX3JlYmFzZWxpbmUoc2VsZiwgcG9ydCk6Ci0gICAgICAg IHJldHVybiBzZWxmLl9leHBlY3RhdGlvbnMocG9ydCkuZ2V0X3JlYmFzZWxpbmluZ19mYWlsdXJl cygpCisgICAgICAgIHJldHVybiBzZWxmLl9leHBlY3RhdGlvbnMocG9ydCwgaW5jbHVkZV9vdmVy cmlkZXM9VHJ1ZSkuZ2V0X3JlYmFzZWxpbmluZ19mYWlsdXJlcygpCiAKICAgICBkZWYgX3JlYmFz ZWxpbmVfcG9ydChzZWxmLCBwb3J0X25hbWUpOgogICAgICAgICBpZiBub3Qgc2VsZi5faXNfc3Vw cG9ydGVkX3BvcnQocG9ydF9uYW1lKToKZGlmZiAtLWdpdCBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0 cHkvdG9vbC9jb21tYW5kcy9yZWJhc2VsaW5lX3VuaXR0ZXN0LnB5IGIvVG9vbHMvU2NyaXB0cy93 ZWJraXRweS90b29sL2NvbW1hbmRzL3JlYmFzZWxpbmVfdW5pdHRlc3QucHkKaW5kZXggNGQ3NjVm NmI0YzUyMzgwMmU2NmMzOTgzZjc0YmQwMDlkNDViZDVlNC4uNmFhZWE4NjMwMGNmNzNjYWQxZjFl Njg3ZGQ0NDZjOGU4YWE0OWJkZCAxMDA2NDQKLS0tIGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS90 b29sL2NvbW1hbmRzL3JlYmFzZWxpbmVfdW5pdHRlc3QucHkKKysrIGIvVG9vbHMvU2NyaXB0cy93 ZWJraXRweS90b29sL2NvbW1hbmRzL3JlYmFzZWxpbmVfdW5pdHRlc3QucHkKQEAgLTI0MCwzICsy NDAsMjAgQEAgTU9DSyBydW5fY29tbWFuZDogWydlY2hvJywgJ3JlYmFzZWxpbmUtdGVzdCcsICdX ZWJraXQgV2luJywgJ3VzZXJzY3JpcHRzL2ltYWdlcy4KIAogICAgICAgICBjb21tYW5kLl90ZXN0 c190b19yZWJhc2VsaW5lID0gbGFtYmRhIHBvcnQ6IFsndXNlcnNjcmlwdHMvYW5vdGhlci10ZXN0 Lmh0bWwnLCAndXNlcnNjcmlwdHMvaW1hZ2VzLnN2ZyddCiAgICAgICAgIE91dHB1dENhcHR1cmUo KS5hc3NlcnRfb3V0cHV0cyhzZWxmLCBjb21tYW5kLmV4ZWN1dGUsIFtNb2NrT3B0aW9ucyhvcHRp bWl6ZT1UcnVlKSwgW10sIHRvb2xdLCBleHBlY3RlZF9sb2dzPWV4cGVjdGVkX2xvZ3Nfd2l0aF9v cHRpbWl6ZSwgZXhwZWN0ZWRfc3RkZXJyPWV4cGVjdGVkX3N0ZGVycl93aXRoX29wdGltaXplKQor CisgICAgZGVmIHRlc3Rfb3ZlcnJpZGVzX2FyZV9pbmNsdWRlZF9jb3JyZWN0bHkoc2VsZik6Cisg ICAgICAgIGNvbW1hbmQgPSBSZWJhc2VsaW5lRXhwZWN0YXRpb25zKCkKKyAgICAgICAgdG9vbCA9 IE1vY2tUb29sKCkKKyAgICAgICAgY29tbWFuZC5iaW5kX3RvX3Rvb2wodG9vbCkKKyAgICAgICAg cG9ydCA9IHRvb2wucG9ydF9mYWN0b3J5LmdldCgnY2hyb21pdW0tbWFjLWxpb24nKQorCisgICAg ICAgICMgVGhpcyB0ZXN0cyB0aGF0IHRoZSBhbnkgdGVzdHMgbWFya2VkIGFzIFJFQkFTRUxJTkUg aW4gdGhlIG92ZXJyaWRlcyBhcmUgZm91bmQsIGJ1dAorICAgICAgICAjIHRoYXQgdGhlIG92ZXJy aWRlcyBkbyBub3QgZ2V0IHdyaXR0ZW4gaW50byB0aGUgbWFpbiBmaWxlLgorICAgICAgICBzZWxm Lm92ZXJyaWRlcyA9ICgnQlVHWCBSRUJBU0VMSU5FIDogdXNlcnNjcmlwdHMvYW5vdGhlci10ZXN0 Lmh0bWwgPSBURVhUXG4nCisgICAgICAgICAgICAgICAgICAgICAgICAgICdCVUdZIDogdXNlcnNj cmlwdHMvdGVzdC5odG1sID0gQ1JBU0hcbicpCisKKyAgICAgICAgcG9ydC5fZmlsZXN5c3RlbS53 cml0ZV90ZXh0X2ZpbGUocG9ydC5wYXRoX3RvX3Rlc3RfZXhwZWN0YXRpb25zX2ZpbGUoKSwgJycp CisgICAgICAgIHBvcnQuX2ZpbGVzeXN0ZW0ud3JpdGVfdGV4dF9maWxlKHBvcnQubGF5b3V0X3Rl c3RzX2RpcigpICsgJy91c2Vyc2NyaXB0cy9hbm90aGVyLXRlc3QuaHRtbCcsICcnKQorICAgICAg ICBwb3J0LnRlc3RfZXhwZWN0YXRpb25zX292ZXJyaWRlcyA9IGxhbWJkYTogc2VsZi5vdmVycmlk ZXMKKyAgICAgICAgc2VsZi5hc3NlcnRFcXVhbHMoY29tbWFuZC5fdGVzdHNfdG9fcmViYXNlbGlu ZShwb3J0KSwgc2V0KFsndXNlcnNjcmlwdHMvYW5vdGhlci10ZXN0Lmh0bWwnXSkpCisgICAgICAg IHNlbGYuYXNzZXJ0RXF1YWxzKHBvcnQuX2ZpbGVzeXN0ZW0ucmVhZF90ZXh0X2ZpbGUocG9ydC5w YXRoX3RvX3Rlc3RfZXhwZWN0YXRpb25zX2ZpbGUoKSksICcnKQo=