Bug 41153 – rebaseline-chromium-webkit-tests should add or remove files to local git repository

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

[Hajime Morrita]

rebaseline-chromium-webkit-tests add/delete files to svn repository (using svn add/delete).
But files are left untracked when on git repository.
They should be added to the repository as with svn.
to force downloading the zip.

[Hajime Morrita]

Created attachment 59730 [details]
patch v0

[Hajime Morrita]

How to test:
- download layout_test_results.zip
- add or remove some file to the zip file, them make the zip http-reachable
- run rebaseline-chromium-webkit-test with "-U" option pontint the zip url.

[WebKit Review Bot]

Attachment 59730 [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:190:  deprecated form of raising exception  [pep8/W602] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm.py:353:  at least two spaces before inline comment  [pep8/E261] [5]
Total errors found: 2 in 4 files


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

[Tony Chang]

I think Victor is the original author of this file.  Maybe he can do an initial review?

[Hajime Morrita]

Created attachment 59732 [details]
patch v1; fix style violation

[Shinichiro Hamaji]

Comment on attachment 59732 [details]
patch v1; fix style violation

Looks great. Putting r- for some nicpicks.

WebKitTools/Scripts/webkitpy/common/checkout/scm.py: 
 +  
I think we need this blank line:

"Separate top-level function and class definitions with two blank lines."

http://www.python.org/dev/peps/pep-0008/


WebKitTools/Scripts/webkitpy/common/checkout/scm.py:349
 +      def _add_directories_recursively(self, path):
I would say _add_parent_directories or something. "recursively" sounds you will add a directory and its descendants.

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:352
 +          parent, here = os.path.split(path)
Cannot we just use os.path.dirname here?

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:349
 +      def _add_directories_recursively(self, path):
Also, I would write a docstring here.

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:458
 +      def pset(self, pname, pvalue, path):
I would use propset as the name of this function.

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:462
 +      def pget(self, pname, path):
propget

WebKitTools/ChangeLog:11
 +          - add "-U" and "-q" options to rebaseline_chromium_webkit_tests.py
This can be done in a separate change, but it's OK for now. Please consider splitting your patch next time.

WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:477
 +                      svn_error = True
This variable should be renamed to scm_error?

WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:478
 +                  if not svn_error and isinstance(self._scm, scm.SVN):
I will comment on this separately.

[Shinichiro Hamaji]

I think we might ask Eric to do a quick review before we land this so I'm adding Eric into the CC list.

Eric, could you tell us what happens if we land PNG without setting svn:mime-type=image/png ? Are there any post-commit hook so we don't need to mind them? If not, are these info important? I think I landed some PNGs without setting this property and no one complained so far.

[Eric Seidel (no email)]

(In reply to comment #7)
> I think we might ask Eric to do a quick review before we land this so I'm adding Eric into the CC list.
> 
> Eric, could you tell us what happens if we land PNG without setting svn:mime-type=image/png ? Are there any post-commit hook so we don't need to mind them? If not, are these info important? I think I landed some PNGs without setting this property and no one complained so far.

git handles mime types automagically.  No need to set svn:mime-type.

[Eric Seidel (no email)]

SVN also handles mime types automagically assuming your system is configured correctly. :)  Which it should be.

[Shinichiro Hamaji]

(In reply to comment #9)
> SVN also handles mime types automagically assuming your system is configured correctly. :)  Which it should be.

Thanks a lot for your clarification! Morita-san, maybe we can just remove the propset/get ?

[Eric Seidel (no email)]

You can see http://svnbook.red-bean.com/en/1.5/svn.advanced.props.html#svn.advanced.props.auto for more information on SVN.

[Victor Wang]

Comment on attachment 59732 [details]
patch v1; fix style violation

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:190
 +          raise NotImplementedError("subclasses must implement")
Changed to "raise NotImplementedError, "subclasses must implement"" to be consistent with others.

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:352
 +          parent, here = os.path.split(path)
os.path.dirname(path)?

WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:475
 +                  if 0 != self._scm.add(expected_fullpath, return_exit_code=True):
What if the file is already under  SVN and here try to add it again?

[Hajime Morrita]

Created attachment 59869 [details]
patch v2

[WebKit Review Bot]

Attachment 59869 [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:191:  deprecated form of raising exception  [pep8/W602] [5]
Total errors found: 1 in 4 files


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

[Hajime Morrita]

Hamaji-san, Eric, Victor, thank you for taking a look!
I updated the patch.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py: 
>  +  
> I think we need this blank line:
Fixed.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:349
>  +      def _add_directories_recursively(self, path):
> I would say _add_parent_directories or something. "recursively" sounds you will add a directory and its descendants.
Fixed.

> 
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:352
>  +          parent, here = os.path.split(path)
> Cannot we just use os.path.dirname here?
Fixed.

> 
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:349
>  +      def _add_directories_recursively(self, path):
> Also, I would write a docstring here.
This line is gone ...

> 
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:458
>  +      def pset(self, pname, pvalue, path):
> I would use propset as the name of this function.
Fixed.

> 
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:462
>  +      def pget(self, pname, path):
> propget
Fixed.

> 
> WebKitTools/ChangeLog:11
>  +          - add "-U" and "-q" options to rebaseline_chromium_webkit_tests.py
> This can be done in a separate change, but it's OK for now. Please consider splitting your patch next time.
Ah, I agree. I'm sorry for mixing these up.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:477
>  +                      svn_error = True
> This variable should be renamed to scm_error?
Fixed.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:478
>  +                  if not svn_error and isinstance(self._scm, scm.SVN):
> I will comment on this separately.
This line is gone.

(In reply to comment #12)
> (From update of attachment 59732 [details])
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:190
>  +          raise NotImplementedError("subclasses must implement")
> Changed to "raise NotImplementedError, "subclasses must implement"" to be consistent with others.
> 
Fixed. This will be complained by ews bot, though.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:352
>  +          parent, here = os.path.split(path)
> os.path.dirname(path)?
Fixed.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:475
>  +                  if 0 != self._scm.add(expected_fullpath, return_exit_code=True):
> What if the file is already under  SVN and here try to add it again?
It' for git. We need to re-add updated files for git.

[Hajime Morrita]

> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:191:  deprecated form of raising exception  [pep8/W602] [5]
> Total errors found: 1 in 4 files
This is intentional. I prefer consistency around the neighboring code than PEP in this case.

[Shinichiro Hamaji]

(In reply to comment #16)
> > Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> > WebKitTools/Scripts/webkitpy/common/checkout/scm.py:191:  deprecated form of raising exception  [pep8/W602] [5]
> > Total errors found: 1 in 4 files
> This is intentional. I prefer consistency around the neighboring code than PEP in this case.

I'd just rewrite all lines which use this style in this module.

[Eric Seidel (no email)]

Comment on attachment 59869 [details]
patch v2

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:528
 +          # path is assumed to be cwd relative?
This is a general problem with this class and part of why we recently add a "run" method which knows how to set the CWD to the webkit root when necessary.

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:575
 +          return self.run_status_and_extract_filenames(self.status_command(), self._status_regexp("D"))
We should probably add a new abstraction for this soon.  self._files_matching_status("D") or similar.  Not needed for this patch.

WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:391
 +      def _shared_test_add_recursively(self):
Eventually I need to figure out a better way to do these "shared" tests.

WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:650
 +          self.scm.propset("svn:mime-type", "application/octet-stream", filepath)
The test would be slightly better if it used an impossible mimetype, so that we knew there was no way SVN would autodetect to that particular type.

WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:485
 +                      _log.warn('  Failed to add baselines to SVN.')
Not quite true anymore. :)

This is a FANTASTIC step forward.  Thank you for doing this!

[Eric Seidel (no email)]

Comment on attachment 59869 [details]
patch v2

WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:910
 +                               help='Supress result HTML viewing')
Typo "Suppress" (at least in american english).

WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:909
 +                               default=False,
Generally I don't bother to list default=False, since default=None automatically and I intentionally avoid treating None different from False in my checks (although I don't know if there is consensus in that approach).

WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:923
 +                               default=(''),
Leaving this out, so that it's None seems better.

[Shinichiro Hamaji]

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:34
 +  import shutil
Do we need this?

I'd r+ this after a few days if no one objects... Ah, Eric has already done :)

[Eric Seidel (no email)]

(In reply to comment #17)
> (In reply to comment #16)
> > > Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> > > WebKitTools/Scripts/webkitpy/common/checkout/scm.py:191:  deprecated form of raising exception  [pep8/W602] [5]
> > > Total errors found: 1 in 4 files
> > This is intentional. I prefer consistency around the neighboring code than PEP in this case.
> 
> I'd just rewrite all lines which use this style in this module.

I totally agree.  I think that should be a separate patch though.  rs=me to do so, you don't even need to post it.

Caution, the reason why those are listed that way is to support pylint (which I'm not sure anyone has run in a while).  I expect pylint can parse other syntaxes, but it will get confused if we move them all into a "throw notimplemented" call for example.

[Hajime Morrita]

Committed r61976: <http://trac.webkit.org/changeset/61976>

[Hajime Morrita]

Hi Eric, Hamaji-san, thank you for reviewing!
I landed the patch, and I'll fix the exception syntax and will tackle Bug 38775 soon.

[WebKit Review Bot]

http://trac.webkit.org/changeset/61977 might have broken GTK Linux 64-bit Debug

[Adam Barth]

I presume you ran "test-webkitpy --all"

The scm unit tests are off by default.

[Kent Tamura]

Would you update https://trac.webkit.org/wiki/Rebaseline please?

[Hajime Morrita]

> Would you update https://trac.webkit.org/wiki/Rebaseline please?
Done. Thank you for pointing this out.