[Webkit-unassigned] [Bug 37630] delete redundant test outputs

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37630





--- Comment #21 from Marcus Bulach <bulach at chromium.org>  2010-08-10 12:51:27 PST ---
thanks Eric! I think I addressed most of your comments, would you mind taking another look please?

(In reply to comment #18)
> (From update of attachment 64026 [details])
> WebKitTools/ChangeLog:1
>  +  2010-08-10  Marcus Bulach  <bulach at chromium.org>
> Should be a new line between the author line and the reviewed by line.
Done

> 
> WebKitTools/Scripts/deduplicate-tests:65
>  +      if options.pretty_print:
> Is this some sort of verbose mode?
yep, changed to verbose.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:47
>  +  def get_port_fallbacks():
> I dont' think we usually use get_* in python code.  Check PEP8.  We certainly don't use get* in our c++ webkit code.
done.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:58
>  +              _log.error("'%s' lacks baseline_search_path()" % port_name)
> Do we ever hit this?  Wouldn't that be a bug in the port implementation?  I think it's OK to be robust here, but we should be more explicit that the port needs fixing.
Clarified the message.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:59
>  +              port_fallbacks[port_name] = ['base']
> base?  Is that a magic string here?
> 
> Seems we might want a constant for that string instead of repeating it.
Added as constant.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:79
>  +      cmd = ('git', 'ls-tree', '-r', 'HEAD', 'LayoutTests')
> I feel like the running of ls tree and returning of the split lines should be a separate funciton.
agreed! split.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:98
>  +          _, _, hash = attrs.split(' ')
> This is a cool pattern, I wish we had understood it better when writing the rest of webkitpy.
:)


> 
> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:89
>  +      for line in git.split('\n'):
> This loop feels like it wants to call some helper function to proces each line.
done.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:113
>  +          if len(cluster) < 2:
> This per-test block feels like a function.  :)  Mostly because I was initially confused as to how you were avoiding deduping the empty file.  But it now seems more clear to me that your'e not looping on hashes, but rather test paths (which is important).

I split this up, let me know if it's any clearer.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:116
>  +          # Compute the list of platforms we have this particular hash for.
> Isn't this a function?  platforms_with_test_matching_hash()?
I named "extract_platforms", hope it's ok?

> 
> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests.py:139
>  +      for test, platform, fallback, path in find_dups(hashes, port_fallbacks):
> If this was a function, then deduplicate just becomes a list comprehension.
yay, more python-foo! :)

> 
> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests_unittest.py:32
>  +  class MockExecutive(object):
> Doesn't Mock.py already do this for you?  We include it in webkitpy/third_party iirc.  It's not great, but it might be easier than rolling your own.
well, I'm not doing anything particularly complicated, so I think it's simpler to keep it?

> 
> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests_unittest.py:57
>  +              '100644 blob 5053240b3353f6eb39f7cb00259785f16d121df2\tLayoutTests/mac/foo-expected.txt\n'
> Would be nice to have a parsing function split out from the logic so we could test that independently.
done.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests_unittest.py:64
>  +          self.assertEquals(('git', 'ls-tree', '-r', 'HEAD', 'LayoutTests'), MockExecutive.last_run_command[-1])
> I see.  Yeah, we don't have quite so slick an interface for this in our existing mocks.  If you look at mocktool.py you'll find a MockExecutive I think.  But the way we've been asserting is using OutputCapture.  Your solution is more pythony, I think.
> 
> We may wish to move some variant of your MockExecutive into a shared file at some point.

I'm happy to move out at any point..

> 
> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests_unittest.py:113
>  +  if __name__ == '__main__':
> I don't think we need this.  Did you verify that test-webkitpy will automagically find and run your unit tests?  I would expect it to.
right, it does find. removed it.

> 
> Overall I think this ia a FANTASTIC patch.  I really can't thank you and evan enough for taking this on.

all credits to Evan, my changes were mostly cosmetic. :)

> 
> I would like to see one more round of this.  Don't feel obliged to answer each of my nits above, but you should at least thumb through them.  I think this patch is OK to land as is, but it would be nice to at least fix the ChangeLog and consider the nits listed.

I think I addressed all of them. another look please?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.