Bug 44709 – deduplicate-tests should be runnable from any WebKit directory

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Tony Chang]

Currently, you have to run it from the root of the checkout.  Otherwise, the following is raised:

  File "/src/WebKit/WebKitTools/Scripts/deduplicate-tests", line 84, in <module>
    main()
  File "/src/WebKit/WebKitTools/Scripts/deduplicate-tests", line 80, in main
    run(options)
  File "/src/WebKit/WebKitTools/Scripts/deduplicate-tests", line 74, in run
    for dupe in deduplicate_tests.deduplicate(options.glob_pattern):
  File "/src/chrome/src/third_party/WebKit.git/WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py", line 195, in deduplicate
    fallbacks = port_fallbacks()
  File "/src/chrome/src/third_party/WebKit.git/WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py", line 55, in port_fallbacks
    for port_name in os.listdir(os.path.join('LayoutTests', 'platform')):
OSError: [Errno 2] No such file or directory: 'LayoutTests/platform'

[Tony Chang]

Created attachment 67116 [details]
Patch

[Tony Chang]

(In reply to comment #1)
> Created an attachment (id=67116) [details]
> Patch

This allows deduplicate-tests to be run from any directory in the checkout and will output relative paths so the results can be piped to rm.

Also, if you run it in a subdir of LayoutTests, it only outputs files under where you run it from.  E.g., if I run the tool while I'm in LayoutTests/win/, I'll only tell me about dupes in LayoutTests/win/.

[Dirk Pranke]

Comment on attachment 67116 [details]
Patch

> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index 1807a838c1a39f7b97f4e184509935cc548d7e37..0d9653bc8c75eb021d002ffc84d9d35386e06d78 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,13 @@
> [phone]  Tony Chang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        deduplicate-tests should be runnable from any WebKit directory
> +        https://bugs.webkit.org/show_bug.cgi?id=44709
> +
> +        * Scripts/webkitpy/layout_tests/deduplicate_tests.py:
> +        * Scripts/webkitpy/layout_tests/deduplicate_tests_unittest.py:
> +
>  2010-09-09  Mihai Parparita  <[e-mail]>
>  
>          Reviewed by Tony Chang.
> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py b/WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py
> index c543d9183a406e78714202e83c99565452357409..beeb680650070a12e0f0f07d8ba80cb9d7860740 100644
> --- a/WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py
> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py
> @@ -36,6 +36,7 @@ import os
>  import subprocess
>  import sys
>  import re
> +import webkitpy.common.checkout.scm as scm
>  import webkitpy.common.system.executive as executive
>  import webkitpy.common.system.logutils as logutils
>  import webkitpy.layout_tests.port.factory as port_factory
> @@ -56,7 +57,8 @@ def port_fallbacks():
>          try:
>              platforms = port_factory.get(port_name).baseline_search_path()
>          except NotImplementedError:
> -            _log.error("'%s' lacks baseline_search_path(), please fix." % port_name)
> +            _log.error("'%s' lacks baseline_search_path(), please fix."
> +                       % port_name)

We don't actually require 80 char lines, but I won't complain if you're 
trying to maintain them anyway (I do).

>              fallbacks[port_name] = [_BASE_PLATFORM]
>              continue
>          fallbacks[port_name] = [os.path.basename(p) for p in platforms][1:]
> @@ -156,11 +158,35 @@ def has_intermediate_results(test, fallbacks, matching_platform,
>      return False
>  
>  
> -def find_dups(hashes, port_fallbacks):
> +def get_relative_test_path(filename, relative_to):
> +    """Constructs a relative path to |filename| from |relative_to|.  Also, if
> +    |relative_to| is a sub directory of the layout test directory and
> +    |filename| is not in |relative_to|, return None.  This lets us filter
> +    the results to only show results that are under where the script was run
> +    from.
> +    Args:
> +        filename: The test file we're trying to get a relative path to.
> +        relative_to: The path we're relative to.

Nit: I might note that relative_to needs to be an absolute path.

Otherwise, LGTM although I'm not a reviewer.

-- Dirk

[Tony Chang]

Created attachment 67120 [details]
Patch

[Tony Chang]

Committed r67216: <http://trac.webkit.org/changeset/67216>

[Tony Chang]

Reverted r67216 for reason:

Broke

Committed r67218: <http://trac.webkit.org/changeset/67218>

[Tony Chang]

(In reply to comment #6)
> Reverted r67216 for reason:
> 
> Broke
> 
> Committed r67218: <http://trac.webkit.org/changeset/67218>

Err, the reason got truncated.  The reason was that relpath is a python2.6+ feature.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67216 might have broken Chromium Mac Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/67216
http://trac.webkit.org/changeset/67215
http://trac.webkit.org/changeset/67205
http://trac.webkit.org/changeset/67206
http://trac.webkit.org/changeset/67207

[Tony Chang]

Created attachment 67245 [details]
Patch

[Tony Chang]

(In reply to comment #9)
> Created an attachment (id=67245) [details]
> Patch

Same as before except I rolled my own relpath.  I tested on Win and Linux and it seems to work.

[Eric Seidel (no email)]

Comment on attachment 67245 [details]
Patch

This seems wrong.

Did you just want this?
http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/system/ospath.py

Why not use absolute paths?

See how scm.py solves the run-from-anywhere problem by passing a cwd to executive.py:

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/checkout/scm.py

[Tony Chang]

Committed r67241: <http://trac.webkit.org/changeset/67241>

[Tony Chang]

(In reply to comment #12)
> Committed r67241: <http://trac.webkit.org/changeset/67241>

Sorry, manual error of webkit-patch.  Reverting . . .

[Tony Chang]

Reverted r67241 for reason:

Accidentally committed

Committed r67242: <http://trac.webkit.org/changeset/67242>

[Tony Chang]

Created attachment 67250 [details]
Patch

[Tony Chang]

Created attachment 67251 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 67251 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67251&action=prettypatch

Looks fine.  The attempts to wrap this to 80c make it harder to read than necessary.

> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:61
> +            _log.error("'%s' lacks baseline_search_path(), please fix."
80c does not help readability in all cases.  But I think I lost that battle long ago.  Cant remember. :)

> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:173
> +    """Constructs a relative path to |filename| from |relative_to|.  Also, if
> +    |relative_to| is a sub directory of the layout test directory and
> +    |filename| is not in |relative_to|, return None.  This lets us filter
> +    the results to only show results that are under where the script was run
> +    from.
> +    Args:
> +        filename: The test file we're trying to get a relative path to.
> +        relative_to: The absolute path we're relative to.
> +    Returns:
> +        A relative path to filename or None.
> +    """
This very long docstring ends up basically saying the same thing 3 times.  Especially with the comment about ospath.relpath below it too. :(

> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:180
> +    path = ospath.relpath(os.path.normpath(abs_path),
> +                          os.path.normpath(relative_to))
> +    return path
Just return the result directly, no?

> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:218
> +                yield {'test': test, 'platform': platform,
> +                       'fallback': fallback, 'path': path}
I would have written this as one per line:

yield {
  'test': foo,
   'bar: bar,
}

[Tony Chang]

Created attachment 67253 [details]
Patch

[Tony Chang]

(In reply to comment #11)
> (From update of attachment 67245 [details])
> Did you just want this?
> http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/system/ospath.py

Switched to using ospath.relpath, but the description is wrong.  It's like python2.6's os.path.relpath, but it returns None if |path| is outside |start_path|.  os.path.relpath is smart enough to put ../ in the returned value.  This is good enough for this script, which will now print nothing if you try to run it from, e.g., WebCore or WebKitTools.

> Why not use absolute paths?

Mainly because it's harder to read and these are really long path names.

> See how scm.py solves the run-from-anywhere problem by passing a cwd to executive.py:

Good idea, I switch to passing the cwd to run_command and using an absolute path the other place it mattered.

[Tony Chang]

Created attachment 67259 [details]
Patch

[Tony Chang]

(In reply to comment #17)
> (From update of attachment 67251 [details])
> This very long docstring ends up basically saying the same thing 3 times.  Especially with the comment about ospath.relpath below it too. :(

Shorted and removed the redundant comment.

> > WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:180
> > +    path = ospath.relpath(os.path.normpath(abs_path),
> > +                          os.path.normpath(relative_to))
> > +    return path
> Just return the result directly, no?

Done.

> > WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:218
> > +                yield {'test': test, 'platform': platform,
> > +                       'fallback': fallback, 'path': path}
> I would have written this as one per line:
> 
> yield {
>   'test': foo,
>    'bar: bar,
> }

Done.

[Eric Seidel (no email)]

Comment on attachment 67259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67259&action=prettypatch

Looks fine, but I think you want to cache find_checkout_root()

> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:111
> +            cwd=scm.find_checkout_root())
find_checkout_root is kinda expensive (hits the disk).  Maybe your object shoudl cache it somewhere?  If this was an actual Command object with a tool.scm, you would just grab tool.scm.checkout_root. :(   Bug 45838 is kinda blocking nice non-webkit-patch webkitpy development in this way.

[Tony Chang]

Committed r67588: <http://trac.webkit.org/changeset/67588>