Bug 40418 – Removes duplication between platform/chromium-win and platform/chromium-mac test expectations by moving files to platform/chromium.

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Marcus Bulach]

Following discussions from:
https://bugs.webkit.org/show_bug.cgi?id=39721
http://code.google.com/p/chromium/issues/detail?id=46142

We need to remove duplication between test expectations on platform/chromium-win and platform/chromium-mac.

This is due the following fallback logic:
platform/chromium-linux => platform/chromium-win -> platform/chromium
and
platform/chromium-mac -> platform/chromium

I used the following one-off script to create this patch:


fdupes -r chromium* | grep -A1 "chromium-win/" | grep -B1 chromium-mac | grep -v '\-\-' > dupes.txt
python << EOF
import os
import shutil
f = []
for l in file('dupes.txt').readlines():
  f.append(l.replace('\n',''))
while len(f):
  if os.path.basename(f[0]) == os.path.basename(f[1]):
    print 'Moving ' + f[0]
    dest = f[0].replace('-win','')
    if not os.path.exists(os.path.dirname(dest)):
      os.makedirs(os.path.dirname(dest))
    shutil.move(f[0], dest)
    os.system('git add ' + dest)
    os.unlink(f[1])
  del f[:2]
EOF

[Marcus Bulach]

Created attachment 58369 [details]
Patch

[Marcus Bulach]

Note: I've left out LayoutTests/platform/chromium-win/fast/js/regexp-overflow-expected.txt as it's 2.1M and was preventing webkit-patch from uploading the diff. I'll upload it as a separate bug.

[Victor Wang]

Comment on attachment 58369 [details]
Patch

> +        * platform/chromium-win/fast/dom/SelectorAPI/README: Removed.
> +        * platform/chromium/fast/dom/SelectorAPI/README: Renamed from LayoutTests/platform/chromium-mac/fast/dom/SelectorAPI/README.
Should README file be removed? Think it would be better to keep README in same folder with the expectation files in this case.

[Marcus Bulach]

Created attachment 58460 [details]
Patch

[Marcus Bulach]

(In reply to comment #3)
> (From update of attachment 58369 [details])
> > +        * platform/chromium-win/fast/dom/SelectorAPI/README: Removed.
> > +        * platform/chromium/fast/dom/SelectorAPI/README: Renamed from LayoutTests/platform/chromium-mac/fast/dom/SelectorAPI/README.
> Should README file be removed? Think it would be better to keep README in same folder with the expectation files in this case.

Good point, thanks Victor! I changed the script a bit so it now only deals with .txt files. Would you mind taking another look and double check it's fine, please?

fdupes -r chromium* | grep -A1 "chromium-win/.*txt" | grep -B1 chromium-mac \
| grep -v "regexp-overflow-expected\.txt" | grep -v '\-\-' > dupes.txt
python << EOF
import os
import shutil
f = []
for l in file('dupes.txt').readlines():
  f.append(l.replace('\n',''))
while len(f):
  if os.path.basename(f[0]) == os.path.basename(f[1]):
    print 'Moving ' + f[0]
    dest = f[0].replace('-win','')
    if not os.path.exists(os.path.dirname(dest)):
      os.makedirs(os.path.dirname(dest))
    shutil.move(f[0], dest)
    os.system('git add ' + dest)
    os.unlink(f[1])
  del f[:2]
EOF

[Victor Wang]

LGTM

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 58369 [details] [details])
> > > +        * platform/chromium-win/fast/dom/SelectorAPI/README: Removed.
> > > +        * platform/chromium/fast/dom/SelectorAPI/README: Renamed from LayoutTests/platform/chromium-mac/fast/dom/SelectorAPI/README.
> > Should README file be removed? Think it would be better to keep README in same folder with the expectation files in this case.
> 
> Good point, thanks Victor! I changed the script a bit so it now only deals with .txt files. Would you mind taking another look and double check it's fine, please?
> 
> fdupes -r chromium* | grep -A1 "chromium-win/.*txt" | grep -B1 chromium-mac \
> | grep -v "regexp-overflow-expected\.txt" | grep -v '\-\-' > dupes.txt
> python << EOF
> import os
> import shutil
> f = []
> for l in file('dupes.txt').readlines():
>   f.append(l.replace('\n',''))
> while len(f):
>   if os.path.basename(f[0]) == os.path.basename(f[1]):
>     print 'Moving ' + f[0]
>     dest = f[0].replace('-win','')
>     if not os.path.exists(os.path.dirname(dest)):
>       os.makedirs(os.path.dirname(dest))
>     shutil.move(f[0], dest)
>     os.system('git add ' + dest)
>     os.unlink(f[1])
>   del f[:2]
> EOF

[Eric Seidel (no email)]

It should be possible to write a generic version of this script using the classes in webkittools/scripts/webkitpy/layout_test/port /  a generic version of this script 
would be HUGELY useful!

[Darin Adler]

Comment on attachment 58460 [details]
Patch

rs=me

The concept seems correct here and I see nothing wrong in the patch.

[Marcus Bulach]

(In reply to comment #7)
> It should be possible to write a generic version of this script using the classes in webkittools/scripts/webkitpy/layout_test/port /  a generic version of this script 
> would be HUGELY useful!

I'm happy to port it over to python, but do you think it's actually going to be used more than once? :) I'm always a bit worried about one-off scripts that get rotten too quickly.

(oh, and just checking: if I do port to python, I should use things like python's md5 rather than depend on external tools such as fdupes, right?)

[Eric Seidel (no email)]

(In reply to comment #9)
> (In reply to comment #7)
> > It should be possible to write a generic version of this script using the classes in webkittools/scripts/webkitpy/layout_test/port /  a generic version of this script 
> > would be HUGELY useful!
> 
> I'm happy to port it over to python, but do you think it's actually going to be used more than once? :) I'm always a bit worried about one-off scripts that get rotten too quickly.
> 
> (oh, and just checking: if I do port to python, I should use things like python's md5 rather than depend on external tools such as fdupes, right?)

We have a bunch of one-off scripts which we have committed to WebKitTools/Scripts and run occasionally.  Most of them are perl, but more and more of our new scripting is moving to python.

I guess I didn't realize this wasn't already python.  It certainly looks like it. :)

I think what you've done is great.  I just would love a script that we can run every 6 months to make sure we don't have duplicate layout test results.  Layout tests are constantly changing, and as the port converge and diverge in their behavior we should be able to easily keep the duplicate result files to a minimum.

An advanced version of such a script could point out that all the ports agree with each other, just that the main expected results should really be mac-only, or some-such.  That's probably a v2 feature though. :)

[Marcus Bulach]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > It should be possible to write a generic version of this script using the classes in webkittools/scripts/webkitpy/layout_test/port /  a generic version of this script 
> > > would be HUGELY useful!
> > 
> > I'm happy to port it over to python, but do you think it's actually going to be used more than once? :) I'm always a bit worried about one-off scripts that get rotten too quickly.
> > 
> > (oh, and just checking: if I do port to python, I should use things like python's md5 rather than depend on external tools such as fdupes, right?)
> 
> We have a bunch of one-off scripts which we have committed to WebKitTools/Scripts and run occasionally.  Most of them are perl, but more and more of our new scripting is moving to python.

cool! I'll take a look  at webkitpy and will rewrite this script using those classes.

> 
> I guess I didn't realize this wasn't already python.  It certainly looks like it. :)

hehe :) yeah, it's python, except the first line which uses "fdupes".. it's trivial to rewrite in python though, only takes probably four lines instead of one.. :)

> 
> I think what you've done is great.  I just would love a script that we can run every 6 months to make sure we don't have duplicate layout test results.  Layout tests are constantly changing, and as the port converge and diverge in their behavior we should be able to easily keep the duplicate result files to a minimum.

sounds a good reason to check it in!
I'm not too familiar with the "checkstyle", but do you think we could add such checks (roughly as a pre-submit) in there?
if that's not abusing the checkstyle :) we'd have an early warning for duplication..

> 
> An advanced version of such a script could point out that all the ports agree with each other, just that the main expected results should really be mac-only, or some-such.  That's probably a v2 feature though. :)

ok, let me start drafting and will send you a patch shortly. thanks!

[Marcus Bulach]

the python script mentioned here was submitted as part of:
https://bugs.webkit.org/show_bug.cgi?id=37630

eric: I'm happy to re-construct this patch using the new script, but given the timezone difference (I'm on GMT), I guess it'd be more effective if you could run the script / review / land in PST? since it's going to be a reasonably broad change, I'm afraid the patch will get stale and we'll have to resolve things if I try to do from this side of the pond.. thoughts?

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #7)
> > > > It should be possible to write a generic version of this script using the classes in webkittools/scripts/webkitpy/layout_test/port /  a generic version of this script 
> > > > would be HUGELY useful!
> > > 
> > > I'm happy to port it over to python, but do you think it's actually going to be used more than once? :) I'm always a bit worried about one-off scripts that get rotten too quickly.
> > > 
> > > (oh, and just checking: if I do port to python, I should use things like python's md5 rather than depend on external tools such as fdupes, right?)
> > 
> > We have a bunch of one-off scripts which we have committed to WebKitTools/Scripts and run occasionally.  Most of them are perl, but more and more of our new scripting is moving to python.
> 
> cool! I'll take a look  at webkitpy and will rewrite this script using those classes.
> 
> > 
> > I guess I didn't realize this wasn't already python.  It certainly looks like it. :)
> 
> hehe :) yeah, it's python, except the first line which uses "fdupes".. it's trivial to rewrite in python though, only takes probably four lines instead of one.. :)
> 
> > 
> > I think what you've done is great.  I just would love a script that we can run every 6 months to make sure we don't have duplicate layout test results.  Layout tests are constantly changing, and as the port converge and diverge in their behavior we should be able to easily keep the duplicate result files to a minimum.
> 
> sounds a good reason to check it in!
> I'm not too familiar with the "checkstyle", but do you think we could add such checks (roughly as a pre-submit) in there?
> if that's not abusing the checkstyle :) we'd have an early warning for duplication..
> 
> > 
> > An advanced version of such a script could point out that all the ports agree with each other, just that the main expected results should really be mac-only, or some-such.  That's probably a v2 feature though. :)
> 
> ok, let me start drafting and will send you a patch shortly. thanks!

[Eric Seidel (no email)]

Anyone can now run the script and delete the unneeded files.  I'm happy to do that tomorrow if no one beats me to it.

[Marcus Bulach]

Comment on attachment 58460 [details]
Patch

sorry, I think this patch as it is should NOT be landed, I created it quite a while ago... we need to run the script again and recreate it.

[Eric Seidel (no email)]

Attachment 58460 [details] was posted by a committer and has review+, assigning to Marcus Voltis Bulach for commit.

[Eric Seidel (no email)]

Comment on attachment 58460 [details]
Patch

Please upload a new version.

[Adam Barth]

This patch is almost surely out of date.  We can do this sort of thing automatically now using "webkit-patch optimize-baselines".

[Adam Barth]

Landed some optimizations in http://trac.webkit.org/changeset/92557