Bug 40792 - Script to list duplicated files, and dedupe some chromium-specific LayoutTests.
Summary: Script to list duplicated files, and dedupe some chromium-specific LayoutTests.
Status: RESOLVED DUPLICATE of bug 37630
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-17 11:52 PDT by Marcus Bulach
Modified: 2010-08-10 02:30 PDT (History)
9 users (show)

See Also:


Attachments
Patch (12.19 KB, patch)
2010-06-17 11:55 PDT, Marcus Bulach
no flags Details | Formatted Diff | Diff
Patch (13.18 KB, patch)
2010-06-18 04:18 PDT, Marcus Bulach
no flags Details | Formatted Diff | Diff
Patch (23.13 KB, patch)
2010-06-21 06:34 PDT, Marcus Bulach
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marcus Bulach 2010-06-17 11:52:04 PDT
Script to list duplicated files, and dedupe some chromium-specific LayoutTests.
Comment 1 Marcus Bulach 2010-06-17 11:55:31 PDT
Created attachment 59025 [details]
Patch
Comment 2 Marcus Bulach 2010-06-17 12:03:37 PDT
Hi Victor, Eric,

Following suggestions from https://bugs.webkit.org/show_bug.cgi?id=40418, here's a couple of scripts to list duplication and move the files around.

Hopefully it'll be useful at some point :), but please, let me know if it's not over-engineered: I tried to separate generic things from the chromium specific bits, and also kept a bunch of what I think it's the standard boiler-plate code here...

Once we're happy with this, I'll rebuild the patch in the change mentioned above using this tool.

Thanks,
Marcus
Comment 3 Victor Wang 2010-06-17 12:18:13 PDT
Thanks Marcus for doing this! How about adding an option "platforms" so this script could be more generic? Now, it hard code chromium-mac and chromium-win. We could use this script to dedup baselines for other ports like chromium-linux, chromium-win-* etc.

(In reply to comment #2)
> Hi Victor, Eric,
> 
> Following suggestions from https://bugs.webkit.org/show_bug.cgi?id=40418, here's a couple of scripts to list duplication and move the files around.
> 
> Hopefully it'll be useful at some point :), but please, let me know if it's not over-engineered: I tried to separate generic things from the chromium specific bits, and also kept a bunch of what I think it's the standard boiler-plate code here...
> 
> Once we're happy with this, I'll rebuild the patch in the change mentioned above using this tool.
> 
> Thanks,
> Marcus
Comment 4 Marcus Bulach 2010-06-18 04:18:12 PDT
Created attachment 59093 [details]
Patch
Comment 5 Marcus Bulach 2010-06-18 04:22:25 PDT
(In reply to comment #3)
> Thanks Marcus for doing this! How about adding an option "platforms" so this script could be more generic? Now, it hard code chromium-mac and chromium-win. We could use this script to dedup baselines for other ports like chromium-linux, chromium-win-* etc.

oh, good point! it made it slightly more complex, but hopefully more useful! :)
it now accepts two parameters: -p for a list of platforms to check for duplicates, and -d for the destination (which may or may not be one of the platforms).
another look please?

> 
> (In reply to comment #2)
> > Hi Victor, Eric,
> > 
> > Following suggestions from https://bugs.webkit.org/show_bug.cgi?id=40418, here's a couple of scripts to list duplication and move the files around.
> > 
> > Hopefully it'll be useful at some point :), but please, let me know if it's not over-engineered: I tried to separate generic things from the chromium specific bits, and also kept a bunch of what I think it's the standard boiler-plate code here...
> > 
> > Once we're happy with this, I'll rebuild the patch in the change mentioned above using this tool.
> > 
> > Thanks,
> > Marcus
Comment 6 Victor Wang 2010-06-18 09:47:13 PDT
Comment on attachment 59093 [details]
Patch

WebKitTools/Scripts/webkitpy/layout_tests/dedupe_chromium_webkit_tests.py:57
 +  
remove the empty line to be consistent with rest of the documentation style?

WebKitTools/Scripts/webkitpy/layout_tests/dedupe_chromium_webkit_tests.py:72
 +          print 'Running: ' + cmd
"_log" instead of "print"? same for other "print".
Comment 7 Adam Barth 2010-06-18 10:07:01 PDT
Comment on attachment 59093 [details]
Patch

webkitpy is for modules.  Scripts go in WebKitTools/Scripts.  In general, we're not accepting new python code without testing.
Comment 8 Adam Barth 2010-06-18 10:08:23 PDT
Also, we don't use os.system.  That API is dangerous.  Please use the APIs in webkitpy/common/executive.py instead.
Comment 9 Adam Barth 2010-06-18 10:09:03 PDT
That will also help you with testing because you can mock out the executive.
Comment 10 Marcus Bulach 2010-06-21 06:34:17 PDT
Created attachment 59247 [details]
Patch
Comment 11 Marcus Bulach 2010-06-21 06:37:46 PDT
thanks Adam and Victor!

the latest patch addresses all your comments so far:
* log instead of print
* moved to WebKitTools/Scripts
* using Executive instead of os.system
* added tests for both scripts

another look please?

(In reply to comment #6)
> (From update of attachment 59093 [details])
> WebKitTools/Scripts/webkitpy/layout_tests/dedupe_chromium_webkit_tests.py:57
>  +  
> remove the empty line to be consistent with rest of the documentation style?
> 
> WebKitTools/Scripts/webkitpy/layout_tests/dedupe_chromium_webkit_tests.py:72
>  +          print 'Running: ' + cmd
> "_log" instead of "print"? same for other "print".

(In reply to comment #9)
> That will also help you with testing because you can mock out the executive.
Comment 12 Marcus Bulach 2010-06-30 10:21:55 PDT
ping? :)

(In reply to comment #11)
> thanks Adam and Victor!
> 
> the latest patch addresses all your comments so far:
> * log instead of print
> * moved to WebKitTools/Scripts
> * using Executive instead of os.system
> * added tests for both scripts
> 
> another look please?
> 
> (In reply to comment #6)
> > (From update of attachment 59093 [details] [details])
> > WebKitTools/Scripts/webkitpy/layout_tests/dedupe_chromium_webkit_tests.py:57
> >  +  
> > remove the empty line to be consistent with rest of the documentation style?
> > 
> > WebKitTools/Scripts/webkitpy/layout_tests/dedupe_chromium_webkit_tests.py:72
> >  +          print 'Running: ' + cmd
> > "_log" instead of "print"? same for other "print".
> 
> (In reply to comment #9)
> > That will also help you with testing because you can mock out the executive.
Comment 13 Victor Wang 2010-06-30 10:36:36 PDT
(In reply to comment #12)
> ping? :)
ok to me, you need a reviewer (maybe abarth) to review this...

> 
> (In reply to comment #11)
> > thanks Adam and Victor!
> > 
> > the latest patch addresses all your comments so far:
> > * log instead of print
> > * moved to WebKitTools/Scripts
> > * using Executive instead of os.system
> > * added tests for both scripts
> > 
> > another look please?
> > 
> > (In reply to comment #6)
> > > (From update of attachment 59093 [details] [details] [details])
> > > WebKitTools/Scripts/webkitpy/layout_tests/dedupe_chromium_webkit_tests.py:57
> > >  +  
> > > remove the empty line to be consistent with rest of the documentation style?
> > > 
> > > WebKitTools/Scripts/webkitpy/layout_tests/dedupe_chromium_webkit_tests.py:72
> > >  +          print 'Running: ' + cmd
> > > "_log" instead of "print"? same for other "print".
> > 
> > (In reply to comment #9)
> > > That will also help you with testing because you can mock out the executive.
Comment 14 Ojan Vafai 2010-07-22 14:35:16 PDT
Comment on attachment 59247 [details]
Patch

A script like this is really useful. Can you extend this to handle checksum and png as well? Also, I would prefer if the script itself knew about the fallback order and just deduped the whole repo instead of requiring the user of the script to do it. For example, if the chromium-mac, chromium and mac expections are all the same, then the two chromium ones should be deleted. I know this is the opposite of what Victor said, but I think we *should* hardcode it. We should just hardcode it to cover all platforms. We can start with just covering some platforms as long as there's a path forward for covering the rest.

In that vein, this isn't just useful for chromium. Can we call the script dedupe_layout_test_expectations.py and put in FIXMEs to cover the other ports?
Comment 15 Eric Seidel (no email) 2010-07-22 14:52:30 PDT
The files in webkitpy/layout_test/port are to be the one true source of fallback information. If they're wrong, we need to fix them. We should definitely use that knowledge from this script.
Comment 16 Tony Chang 2010-08-02 14:02:11 PDT
I hate to be the n-th person to ask for more stuff, but I agree with Eric that we should use the existing fallback code in webkitpy/layout_test/port.  Fortunately, Evan already wrote code for this here (get_port_fallbacks()):
  https://bugs.webkit.org/show_bug.cgi?id=37630

I think once we get that fixed, we can commit this and add the other features requested (checksum files, png files, fast git support, etc etc etc).
Comment 17 Eric Seidel (no email) 2010-08-06 13:10:28 PDT
I'm confused as to how this differs from bug 37630.
Comment 18 Marcus Bulach 2010-08-09 04:54:20 PDT
Hi all,

Apologies for the delay..
Thanks for pointing out https://bugs.webkit.org/show_bug.cgi?id=37630, I wasn't aware of it.

I'll coordinate with Evan, so we can go ahead with either version: either I'll copy his paltform-fallback, or he can re-use some of the tests I've written.

Either way, I hope there'll be some solution soon.. :)


(In reply to comment #17)
> I'm confused as to how this differs from bug 37630.
Comment 19 Marcus Bulach 2010-08-09 11:42:03 PDT
(In reply to comment #18)
> Hi all,
> 
> Apologies for the delay..
> Thanks for pointing out https://bugs.webkit.org/show_bug.cgi?id=37630, I wasn't aware of it.
> 
> I'll coordinate with Evan, so we can go ahead with either version: either I'll copy his paltform-fallback, or he can re-use some of the tests I've written.
> 
> Either way, I hope there'll be some solution soon.. :)
> 
> 
> (In reply to comment #17)
> > I'm confused as to how this differs from bug 37630.

just had Evan's reply, I'll take over his patch (much simpler than this), add the necessary tests / features and send the code for review at 37630.

I'll add all of you to that bug once I got the new patch out.

thanks!
Comment 20 Marcus Bulach 2010-08-10 02:30:03 PDT
Closing this as a duplicate of https://bugs.webkit.org/show_bug.cgi?id=37630.

*** This bug has been marked as a duplicate of bug 37630 ***