Bug 44653 – don't delete duplicates needed because of intermediate results

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Tony Chang]

don't delete duplicates needed because of intermediate results

[Tony Chang]

Created attachment 65506 [details]
Patch

[Tony Chang]

Maybe evan or dirk can casually review.

[Dirk Pranke]

Comment on attachment 65506 [details]
Patch

> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index 63e2743fd4f26c1479d22defadf9e794fab972ff..e7e0440c172174c1495c85bc6da5c93e205564bd 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,15 @@
> [phone]  Tony Chang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        don't delete duplicates needed because of intermediate results
> +        https://bugs.webkit.org/show_bug.cgi?id=44653
> +
> +        Also, output the full path so we can pipe the output to rm.
> +
> +        * Scripts/webkitpy/layout_tests/deduplicate_tests.py:
> +        * Scripts/webkitpy/layout_tests/deduplicate_tests_unittest.py:
> +
>  2010-08-25  Dirk Pranke  <[e-mail]>
>  
>          Reviewed by Ojan Vafai.
> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py b/WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py
> index bb63f5e66930f890d739456d42a833507e4a4681..6c5006ef5924854f5aab71d3defa6c92f2a54809 100644
> --- a/WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py
> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py
> @@ -130,6 +130,30 @@ def extract_platforms(paths):
>      return platforms
>  
>  
> +def has_intermediate_results(test, fallbacks, matching_platform,
> +                             path_exists=os.path.exists):
> +    """Returns True if there is a test result that causes us to not delete
> +    this duplicate.
> +
> +    For example, chromium-linux may be a duplicate of the checked in result,
> +    but chromium-win may have a different result checked in.  In this case,
> +    we need to keep the duplicate results.
> +
> +    Args:
> +        test: The test name.
> +        fallbacks: A list of platforms we fall back on.
> +        matching_platform: The platform that we found the duplicate test
> +            result.  We can stop checking here.
> +    """

It would be nice if you added path_exists to the docstring arg list here, if for no other reason that to clarify that it should only be overridden for unit testing (I'm assuming that's the purpose)? Personally I'd be tempted to mock out os.path.exists() in the unittests themselves and not include the parameter here, but no big deal.


WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:183
 +                          yield test, platform, fallback, path

Why is this path and not platforms[platform] like it was before? Why are you adding "LayoutTests"?

WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests_unittest.py:120
 +                  deduplicate_tests.has_intermediate_results(*inputs))
It might be nice if you added a comment about how this series of tests works, and why this set of combinations is exhaustive (or how it was arrived at). I find people don't tend to document unit tests very clearly, which aggrevates an already difficult maintenance task.

[Tony Chang]

Created attachment 65577 [details]
Patch

[Tony Chang]

(In reply to comment #3)
> It would be nice if you added path_exists to the docstring arg list here, if for no other reason that to clarify that it should only be overridden for unit testing (I'm assuming that's the purpose)? Personally I'd be tempted to mock out os.path.exists() in the unittests themselves and not include the parameter here, but no big deal.

Added a docstring.  I had tried mocking deduplicate_tests.os.path.exists in the unittest, but the change persisted if the test failed.  It seemed dangerous, so I went with the dependency injection pattern instead.

> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:183
>  +                          yield test, platform, fallback, path
> 
> Why is this path and not platforms[platform] like it was before? Why are you adding "LayoutTests"?

I mentioned in the ChangeLog this is so you can pipe the results to 'rm'.

> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests_unittest.py:120
>  +                  deduplicate_tests.has_intermediate_results(*inputs))
> It might be nice if you added a comment about how this series of tests works, and why this set of combinations is exhaustive (or how it was arrived at). I find people don't tend to document unit tests very clearly, which aggrevates an already difficult maintenance task.

I added comments for each test case.  Did you want something more general?

[Dirk Pranke]

(In reply to comment #5)
> (In reply to comment #3)
> > It would be nice if you added path_exists to the docstring arg list here, if for no other reason that to clarify that it should only be overridden for unit testing (I'm assuming that's the purpose)? Personally I'd be tempted to mock out os.path.exists() in the unittests themselves and not include the parameter here, but no big deal.
> 
> Added a docstring.  I had tried mocking deduplicate_tests.os.path.exists in the unittest, but the change persisted if the test failed.  It seemed dangerous, so I went with the dependency injection pattern instead.
> 

You can either wrap the code in a try/finally (see test_reset_results() in run_webkit_tests_unittest, or use a tearDown fixture (see port/factory_unittest); both of those approaches are (I think) reliable.

But, the dependency injection approach is fine, too.

> > WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:183
> >  +                          yield test, platform, fallback, path
> > 
> > Why is this path and not platforms[platform] like it was before? Why are you adding "LayoutTests"?
> 
> I mentioned in the ChangeLog this is so you can pipe the results to 'rm'.
> 

But doesn't that only work if you're running from the top of the checkout? Wouldn't the previous version work just as well if you were running from the LayoutTests dir?

I didn't notice the ChangeLog comment before, but I normally think of "full path" as "abspath", so I would think you'd want something like os.path.join(webkitpy.checkout.scm.find_checkout_path(), "LayoutTests", ...) instead, no?

> > WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests_unittest.py:120
> >  +                  deduplicate_tests.has_intermediate_results(*inputs))
> > It might be nice if you added a comment about how this series of tests works, and why this set of combinations is exhaustive (or how it was arrived at). I find people don't tend to document unit tests very clearly, which aggrevates an already difficult maintenance task.
> 
> I added comments for each test case.  Did you want something more general?

Nope, looks good.

[Tony Chang]

(In reply to comment #6)
> But doesn't that only work if you're running from the top of the checkout? Wouldn't the previous version work just as well if you were running from the LayoutTests dir?

It seems like the test has to be run from WebKit/.  If you run it from the LayoutTests dir, I get a python error.  I can try to fix that in a different change, but for now, it's not possible to run the script and pipe the results to xargs rm.

[Dirk Pranke]

(In reply to comment #7)
> (In reply to comment #6)
> > But doesn't that only work if you're running from the top of the checkout? Wouldn't the previous version work just as well if you were running from the LayoutTests dir?
> 
> It seems like the test has to be run from WebKit/.  If you run it from the LayoutTests dir, I get a python error.  I can try to fix that in a different change, but for now, it's not possible to run the script and pipe the results to xargs rm.

Okay. Well, piping the results to rm is certainly useful, so this sounds like an improvement. It would be nice to file a separate bug so we don't forget the other enhancement.

LGTM otherwise (but I'm not a reviewer).

[Tony Chang]

I filed bug 44709, which I will fix in a follow up as Dirk requested.  Eric, can you review?

[Ojan Vafai]

Comment on attachment 65577 [details]
Patch

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

Does this do the right thing if the intermediate result is the same as the current result? e.g. what if there are results for a test in each of platform/mac, platform-mac-leopard and platform-mac-chromium-leopard, but they're all the same? The ones the two leopard specific results should be listed as redundant, right?

It's hard for me to tell from this code whether this does that or not. Can you add a test for this case? Otherwise, this looks good.

[Tony Chang]

(In reply to comment #10)
> (From update of attachment 65577 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=65577&action=prettypatch
> 
> Does this do the right thing if the intermediate result is the same as the current result? e.g. what if there are results for a test in each of platform/mac, platform-mac-leopard and platform-mac-chromium-leopard, but they're all the same? The ones the two leopard specific results should be listed as redundant, right?
> 
> It's hard for me to tell from this code whether this does that or not. Can you add a test for this case? Otherwise, this looks good.

Only kind of.  I had discussed this with evan.  If A fallsback to B fallsback to C and they all the same result, the first time you run deduplicate-results, A won't be removed because B exists, but B will be removed.  The second time you run it, A will be removed.  This isn't ideal, but it simplifies the logic and this is a relatively rare case.

I've filed bug 45463 about this and will fix in a follow up.

[Tony Chang]

Committed r67092: <http://trac.webkit.org/changeset/67092>