Bug 108577

Created attachment 185938 [details] work in progress

Created attachment 186127 [details] Work in progress 2

Created attachment 187337 [details] Work in progress 3

Created attachment 187412 [details] Work in progress 4

Created attachment 189670 [details] Patch

Attachment 189670 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/common/checkout/scm/git.py', u'Tools/Scripts/webkitpy/common/checkout/scm/scm.py', u'Tools/Scripts/webkitpy/common/checkout/scm/scm_mock.py', u'Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py', u'Tools/Scripts/webkitpy/common/checkout/scm/svn.py', u'Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py', u'Tools/Scripts/webkitpy/layout_tests/port/base.py', u'Tools/Scripts/webkitpy/layout_tests/port/chromium.py', u'Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py', u'Tools/Scripts/webkitpy/performance_tests/perftestsrunner_integrationtest.py', u'Tools/Scripts/webkitpy/performance_tests/perftestsrunner_unittest.py']" exit_code: 1 Tools/Scripts/webkitpy/performance_tests/perftestsrunner_unittest.py:167: [MainTest.test_upload_json.MockFileUploader.__init__] Method should have "self" as first argument [pylint/E0213] [5] Tools/Scripts/webkitpy/performance_tests/perftestsrunner_unittest.py:172: [MainTest.test_upload_json.MockFileUploader.upload_single_text_file] Method should have "self" as first argument [pylint/E0213] [5] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 189670 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189670&action=review > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:367 > - repos.append(('chromium', self.path_from_chromium_base('build'))) > + repos.append(('Chromium', self.path_from_chromium_base('build'))) These are paths? Are you sure you wnat to capitalize here? > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:281 > + url = 'http://trac.webkit.org/browser/trunk/PerformanceTests/' + '/'.join(path[0:i + 1]) You might separate this out into another function. I think common.config.urls already has this anyway. :)

I'd prefer you ask dpranke or ojan for a review. I think they probably know this section of webkitpy better than I do.

(In reply to comment #7) > (From update of attachment 189670 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189670&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:367 > > - repos.append(('chromium', self.path_from_chromium_base('build'))) > > + repos.append(('Chromium', self.path_from_chromium_base('build'))) > > These are paths? Are you sure you wnat to capitalize here? No, they're repository names.

Comment on attachment 189670 [details] Patch Mostly minor comments ... I'd like to review it again briefly once you can get things cleaned up? View in context: https://bugs.webkit.org/attachment.cgi?id=189670&action=review > Tools/ChangeLog:12 > + a svn checkout or a git clone. This information is embedded in JSON submitted to perf.webkit.org. I saw there was some discussion of this on #irc, but can you add a bit more as to *why* you need/want the timestamp? Also, reflecting what we discussed on #webkit, can you put in something about why we need to support two formats, and what the transition plan is? this patch makes things pretty ugly, so it would be good to clarify that this is hopefully temporary. > Tools/ChangeLog:16 > + of this feature has been updated to explicitly lowercase the names. Is this actually needed as a part of this change? AFAIK, these repo names aren't really user-visible and this kinda seems like busy work. I'm not saying you should remove it, but I wouldn't have made the changes unless they were actually needed, especially since you're having to work-around the changes to keep from breaking clients. > Tools/Scripts/webkitpy/common/checkout/scm/git.py:257 > + def svn_timestamp(self, path): Why is this "svn_timestamp" rather than just "timestamp" or "timestamp_of_latest_commit"? Why do you need '-25' rather than just '-1' ? It doesn't look like this is actually filtering for git-svn-id: or anything to find the svn-specific revisions? Do you need to do anything here to ensure you're not pulling timestamps off a branch? > Tools/Scripts/webkitpy/layout_tests/port/base.py:1100 > + By default it returns a list that only contains a ('WebKit', <webkitRepossitoryPath>) tuple.""" typo: webkitRepossitory . I know you didn't add it, but might as well fix it while you're here :). >> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:367 >> + repos.append(('Chromium', self.path_from_chromium_base('build'))) > > These are paths? Are you sure you wnat to capitalize here? The first arg is just an identifier, not a path. > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:208 > def _generate_and_show_results(self): this function is too long and doing too much now ... can you add some FIXMEs to break it up to be easier to follow (and test)? > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:265 > + result = self._results[metric_full_name] nit: for metric_full_name, result in self._results.iteritems(): ? > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:267 > + # We can't reprot results without indivisual measurements. typos. > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:327 > + def _generate_output_files(self, output_json_path, perf_webkit_json_path, results_page_path, output, perf_webkit_output): seems like here you could just call _generate_output_files twice, once with the old output, and then the second time w/ the new output and results_page_path=None ?

(In reply to comment #10) > > > Tools/ChangeLog:12 > > + a svn checkout or a git clone. This information is embedded in JSON submitted to perf.webkit.org. > > I saw there was some discussion of this on #irc, but can you add a bit more as to *why* you need/want the timestamp? > > Also, reflecting what we discussed on #webkit, can you put in something about why we need to support two formats, and what the transition plan is? this patch makes things pretty ugly, so it would be good to clarify that this is hopefully temporary. Done. > > Tools/ChangeLog:16 > > + of this feature has been updated to explicitly lowercase the names. > > Is this actually needed as a part of this change? AFAIK, these repo names aren't really user-visible and this kinda seems like busy work. I'm not saying you should remove it, but I wouldn't have made the changes unless they were actually needed, especially since you're having to work-around the changes to keep from breaking clients. Yes. These names are recorded and exposed on perf.webkit.org as a part of its UI. > > Tools/Scripts/webkitpy/common/checkout/scm/git.py:257 > > + def svn_timestamp(self, path): > > Why is this "svn_timestamp" rather than just "timestamp" or "timestamp_of_latest_commit"? Renamed. > Why do you need '-25' rather than just '-1' ? It doesn't look like this is actually filtering for git-svn-id: or anything to find the svn-specific revisions? Oops, changed it to -1. > Do you need to do anything here to ensure you're not pulling timestamps off a branch? I don't really care what happens on beaches at least for now since none of our bots use branches. > > Tools/Scripts/webkitpy/layout_tests/port/base.py:1100 > > + By default it returns a list that only contains a ('WebKit', <webkitRepossitoryPath>) tuple.""" > > typo: webkitRepossitory . I know you didn't add it, but might as well fix it while you're here :). Fixed. > > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:208 > > def _generate_and_show_results(self): > > this function is too long and doing too much now ... can you add some FIXMEs to break it up to be easier to follow (and test)? Added. > > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:265 > > + result = self._results[metric_full_name] > > nit: for metric_full_name, result in self._results.iteritems(): ? Fixed. > > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:267 > > + # We can't reprot results without indivisual measurements. > > typos. Fixed. > > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:327 > > + def _generate_output_files(self, output_json_path, perf_webkit_json_path, results_page_path, output, perf_webkit_output): > > seems like here you could just call _generate_output_files twice, once with the old output, and then the second time w/ the new output and results_page_path=None ? No. results page should only be generated with the old JSON format for now. I'll update the results page's template to use the new format once this patch is in.

Created attachment 189843 [details] Fixed per comments

Attachment 189843 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/common/checkout/scm/git.py', u'Tools/Scripts/webkitpy/common/checkout/scm/scm.py', u'Tools/Scripts/webkitpy/common/checkout/scm/scm_mock.py', u'Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py', u'Tools/Scripts/webkitpy/common/checkout/scm/svn.py', u'Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py', u'Tools/Scripts/webkitpy/layout_tests/port/base.py', u'Tools/Scripts/webkitpy/layout_tests/port/chromium.py', u'Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py', u'Tools/Scripts/webkitpy/performance_tests/perftestsrunner_integrationtest.py', u'Tools/Scripts/webkitpy/performance_tests/perftestsrunner_unittest.py']" exit_code: 1 Tools/Scripts/webkitpy/performance_tests/perftestsrunner_unittest.py:167: [MainTest.test_upload_json.MockFileUploader.__init__] Method should have "self" as first argument [pylint/E0213] [5] Tools/Scripts/webkitpy/performance_tests/perftestsrunner_unittest.py:172: [MainTest.test_upload_json.MockFileUploader.upload_single_text_file] Method should have "self" as first argument [pylint/E0213] [5] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 189843 [details] Fixed per comments View in context: https://bugs.webkit.org/attachment.cgi?id=189843&action=review r+ with the typos fixed. > Tools/ChangeLog:9 > + --test-results-server. The new format is needed to provide of extra information perf.webkit.org nit: s/provide of/provide/ > Tools/ChangeLog:12 > + webkit-perf.appspot.com will be deleted once the transition is completed. webkit-perf.appspot.com can be deleted. > Tools/ChangeLog:14 > + This patch adds scm.svn_timestamp to obtain the timestamp of the latest commit present in rename to timestamp_of_latest_commit() in the Changelog? > Tools/Scripts/webkitpy/common/checkout/scm/git.py:263 > + # Manually modify the timezone since Git doesn't have an optiton to show it in UTC. typo: "optiton" > Tools/Scripts/webkitpy/common/checkout/scm/git.py:270 > + return time_wihout_timezone.strftime('%Y-%m-%dT%H:%M:%SZ') typo: "wihout" (both lines)

Also, can you make sure that check-webkit-style (and/or lint-webkitpy) is clean, since the style bot seems to be choking?

Created attachment 189848 [details] Patch for landing

(In reply to comment #16) > Created an attachment (id=189848) [details] > Patch for landing I'm going to land this patch manually. I need to prepare both perf.webkit.org and webkit-perf.appsot.com for this patch so please don't land this patch until I do tonight.

Comment on attachment 189848 [details] Patch for landing Clearing flags on attachment: 189848 Committed r143833: <http://trac.webkit.org/changeset/143833>

All reviewed patches have been landed. Closing bug.