Bug 41153 - rebaseline-chromium-webkit-tests should add or remove files to local git repository
Summary: rebaseline-chromium-webkit-tests should add or remove files to local git repo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-24 05:28 PDT by Hajime Morrita
Modified: 2010-06-29 01:23 PDT (History)
7 users (show)

See Also:


Attachments
patch v0 (15.84 KB, patch)
2010-06-24 21:29 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v1; fix style violation (15.85 KB, patch)
2010-06-24 22:05 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v2 (16.28 KB, patch)
2010-06-27 20:04 PDT, Hajime Morrita
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2010-06-24 05:28:30 PDT
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.
Comment 1 Hajime Morrita 2010-06-24 21:29:31 PDT
Created attachment 59730 [details]
patch v0
Comment 2 Hajime Morrita 2010-06-24 21:31:31 PDT
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.
Comment 3 WebKit Review Bot 2010-06-24 21:31:39 PDT
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.
Comment 4 Tony Chang 2010-06-24 22:04:10 PDT
I think Victor is the original author of this file.  Maybe he can do an initial review?
Comment 5 Hajime Morrita 2010-06-24 22:05:31 PDT
Created attachment 59732 [details]
patch v1; fix style violation
Comment 6 Shinichiro Hamaji 2010-06-24 22:51:42 PDT
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.
Comment 7 Shinichiro Hamaji 2010-06-24 22:56:41 PDT
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.
Comment 8 Eric Seidel (no email) 2010-06-24 23:05:07 PDT
(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.
Comment 9 Eric Seidel (no email) 2010-06-24 23:07:24 PDT
SVN also handles mime types automagically assuming your system is configured correctly. :)  Which it should be.
Comment 10 Shinichiro Hamaji 2010-06-24 23:09:26 PDT
(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 ?
Comment 11 Eric Seidel (no email) 2010-06-24 23:13:41 PDT
You can see http://svnbook.red-bean.com/en/1.5/svn.advanced.props.html#svn.advanced.props.auto for more information on SVN.
Comment 12 Victor Wang 2010-06-25 09:37:27 PDT
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?
Comment 13 Hajime Morrita 2010-06-27 20:04:05 PDT
Created attachment 59869 [details]
patch v2
Comment 14 WebKit Review Bot 2010-06-27 20:06:30 PDT
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.
Comment 15 Hajime Morrita 2010-06-27 20:08:41 PDT
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.
Comment 16 Hajime Morrita 2010-06-27 20:10:07 PDT
> 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.
Comment 17 Shinichiro Hamaji 2010-06-27 21:11:21 PDT
(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.
Comment 18 Eric Seidel (no email) 2010-06-27 21:16:22 PDT
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!
Comment 19 Eric Seidel (no email) 2010-06-27 21:18:44 PDT
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.
Comment 20 Shinichiro Hamaji 2010-06-27 21:20:58 PDT
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 :)
Comment 21 Eric Seidel (no email) 2010-06-27 21:24:42 PDT
(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.
Comment 22 Hajime Morrita 2010-06-27 22:16:13 PDT
Committed r61976: <http://trac.webkit.org/changeset/61976>
Comment 23 Hajime Morrita 2010-06-27 22:17:45 PDT
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.
Comment 24 WebKit Review Bot 2010-06-27 23:15:03 PDT
http://trac.webkit.org/changeset/61977 might have broken GTK Linux 64-bit Debug
Comment 25 Adam Barth 2010-06-28 00:19:25 PDT
I presume you ran "test-webkitpy --all"

The scm unit tests are off by default.
Comment 26 Kent Tamura 2010-06-28 21:03:18 PDT
Would you update https://trac.webkit.org/wiki/Rebaseline please?
Comment 27 Hajime Morrita 2010-06-29 01:23:48 PDT
> Would you update https://trac.webkit.org/wiki/Rebaseline please?
Done. Thank you for pointing this out.