Bug 40792 – Script to list duplicated files, and dedupe some chromium-specific LayoutTests.

• Component: New Bugs
• Status: RESOLVED DUPLICATE

[Marcus Bulach]

Script to list duplicated files, and dedupe some chromium-specific LayoutTests.

[Marcus Bulach]

Created attachment 59025 [details]
Patch

[Marcus Bulach]

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

[Victor Wang]

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

[Marcus Bulach]

Created attachment 59093 [details]
Patch

[Marcus Bulach]

(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

[Victor Wang]

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".

[Adam Barth]

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.

[Adam Barth]

Also, we don't use os.system.  That API is dangerous.  Please use the APIs in webkitpy/common/executive.py instead.

[Adam Barth]

That will also help you with testing because you can mock out the executive.

[Marcus Bulach]

Created attachment 59247 [details]
Patch

[Marcus Bulach]

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.

[Marcus Bulach]

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.

[Victor Wang]

(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.

[Ojan Vafai]

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?

[Eric Seidel (no email)]

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.

[Tony Chang]

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).

[Eric Seidel (no email)]

I'm confused as to how this differs from bug 37630.

[Marcus Bulach]

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.

[Marcus Bulach]

(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!

[Marcus Bulach]

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 ***