Bug 38775 – rebaseline-chromium-webkit-tests doesn't do diffs right with a Git checkout of WebKit

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

[Dirk Pranke]

if you run rebaseline-chromium-webkit-tests using a Git checkout of WebKit, it can't produce the correct results.html file to show the diffs - it assumes you can do an 'svn cat' of the old -expected file.

See

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py#L697

in particular around line 719.

[Adam Barth]

This script should use scm.py to abstract away the differences between svn and git to avoid this entire class of bugs instead of re-inventing scmy.py as it does now.

[Dirk Pranke]

Created attachment 55425 [details]
Patch

[Dirk Pranke]

here's a minimal patch to make things work in Git; we should really abstract all the SCM stuff out though. There's probably code already written to do this.

Also, we should diff the two files directly rather using the port routines rather than using svn diff. Haven't made that change yet.

[Dirk Pranke]

whoops; didn't see Adam's comment, but I agree completely.

[Victor Wang]

(In reply to comment #4)
> whoops; didn't see Adam's comment, but I agree completely.

Agreed. There are a couple of things I like to refactor the rebaseline tool after it has been upstreamed. Working with git (the tool only works with svn when it was written last year) is one item on the list, other items like better summarized messages, better working with the upstream infrastructure (port, multiple test_expectations etc). I have some other tasks on my Q2 list, but should be able to get into this in later Q2 or earlier Q3.

[Hajime Morrita]

Created attachment 59887 [details]
patch v0

[Hajime Morrita]

As we introduce SCM abstraction to rebaseline_chromium_webkit_test,
now we can replace diff and cat with them. Here is an attempt.

[Victor Wang]

Comment on attachment 59887 [details]
patch v0

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:441
 +              self._bogus_dir = os.path.join(parent_dir, "temp_svn_config")
Should _bogus_dir be created for win?

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:462
 +              return self.run(['svn', 'diff', path])
Missing "--config-dir"?

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:467
 +          return self.run(['svn', 'cat', path])
Add "-r BASE" to svn cat?

[Hajime Morrita]

Created attachment 59986 [details]
patch v1

[Hajime Morrita]

Hi Victor, thank you for reviewing!
I updated the patch.

(In reply to comment #8)
> (From update of attachment 59887 [details])
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:441
>  +              self._bogus_dir = os.path.join(parent_dir, "temp_svn_config")
> Should _bogus_dir be created for win?
Fixed. (I broke the code during copying...)

> 
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:462
>  +              return self.run(['svn', 'diff', path])
> Missing "--config-dir"?
Fixed.

> 
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:467
>  +          return self.run(['svn', 'cat', path])
> Add "-r BASE" to svn cat?
Fixed, and added test for SVN.show_head()

[Victor Wang]

Ok to me, you need a reviewer to review this.

(In reply to comment #10)
> Hi Victor, thank you for reviewing!
> I updated the patch.
> 
> (In reply to comment #8)
> > (From update of attachment 59887 [details] [details])
> > WebKitTools/Scripts/webkitpy/common/checkout/scm.py:441
> >  +              self._bogus_dir = os.path.join(parent_dir, "temp_svn_config")
> > Should _bogus_dir be created for win?
> Fixed. (I broke the code during copying...)
> 
> > 
> > WebKitTools/Scripts/webkitpy/common/checkout/scm.py:462
> >  +              return self.run(['svn', 'diff', path])
> > Missing "--config-dir"?
> Fixed.
> 
> > 
> > WebKitTools/Scripts/webkitpy/common/checkout/scm.py:467
> >  +          return self.run(['svn', 'cat', path])
> > Add "-r BASE" to svn cat?
> Fixed, and added test for SVN.show_head()

[Shinichiro Hamaji]

Comment on attachment 59986 [details]
patch v1

Cannot we test diff_for_file?

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:228
 +      def diff_for_file(self, path, optios={}):
Hmm... I'd prefer using keyword arguments for all possible options.

def diff_for_file(self, use_bogus_dir=True, git_cached=True, ...)

so we won't make typo. I think this way is more consistent with other code in this file (see changed_file for example)

By the way, do we really need these options?

[Hajime Morrita]

Created attachment 60197 [details]
patch v2

[WebKit Review Bot]

Attachment 60197 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/Scripts/webkitpy/common/checkout/scm.py:433:  trailing whitespace  [pep8/W291] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:1172:  trailing whitespace  [pep8/W291] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:1187:  trailing whitespace  [pep8/W291] [5]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Shinichiro Hamaji]

Comment on attachment 60197 [details]
patch v2

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:455
 +      def diff_for_file(self, path, use_bogus_dir=False, log=None, **kwargs):
Still this code allows typo. I'd just remove kwargs.

[Hajime Morrita]

Created attachment 60198 [details]
patch v2.1; fixed style violation

[Hajime Morrita]

Created attachment 60202 [details]
patch v3; removed redundant options

[Hajime Morrita]

Hi Hamaji-san, thank you reviewing again.
I updated the patch.

> Still this code allows typo. I'd just remove kwargs.
Ah, I get what you meant to say.
And I removed options which can be altered by appropriate default behavior.

[Shinichiro Hamaji]

Comment on attachment 60202 [details]
patch v3; removed redundant options

Looks good. Thanks for your update!

[Hajime Morrita]

Committed r62236: <http://trac.webkit.org/changeset/62236>