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.
Created attachment 59730 [details] patch v0
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.
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.
I think Victor is the original author of this file. Maybe he can do an initial review?
Created attachment 59732 [details] patch v1; fix style violation
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.
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.
(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.
SVN also handles mime types automagically assuming your system is configured correctly. :) Which it should be.
(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 ?
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 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?
Created attachment 59869 [details] patch v2
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.
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.
> 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.
(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 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 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.
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 :)
(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.
Committed r61976: <http://trac.webkit.org/changeset/61976>
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.
http://trac.webkit.org/changeset/61977 might have broken GTK Linux 64-bit Debug
I presume you ran "test-webkitpy --all" The scm unit tests are off by default.
Would you update https://trac.webkit.org/wiki/Rebaseline please?
> Would you update https://trac.webkit.org/wiki/Rebaseline please? Done. Thank you for pointing this out.