Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
536553003:
Parse Git hash for dependency repositories from DEPS file. (Closed)
DescriptionParse Git hash for dependency repositories from DEPS file.
BUG=409875
NOTRY=true
Committed: https://crrev.com/87fdd0ac5c9604e2ef3af7cd845ac254b7dd51b8
Cr-Commit-Position: refs/heads/master@{#293351}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 12
Patch Set 5 : #Patch Set 6 : #
Total comments: 8
Patch Set 7 : #
Total comments: 2
Messages
Total messages: 16 (2 generated)
prasadv@chromium.org changed reviewers: + qyearsley@chromium.org
https://codereview.chromium.org/536553003/diff/40001/tools/bisect-perf-regres... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/536553003/diff/40001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:1443: r'(?<=[",\']%s[",\']: [",\'])([a-fA-F0-9]{40})(?=[",\'])' % 1. The character class [",\'] in this regex matches commas as well: =>>> re.match(r'[",\']12345[",\']', ',12345,').group(0) => ',12345,' 2. The lookahead (?=...) and lookbehind (?<=...) assertions aren't necessary; as long as the main part that we want to capture is in a group, then we can capture it. =>>> re.match(r'["\']([0-9]{5})["\']', '"12345"').group(1) => '12345' So, I think the regex maybe should be: r'["\']%s["\']: [",\']([a-fA-F0-9]{40})["\']' % deps_var Also, this regex doesn't guarantee that the quotes match. =>>> re.match(r'["\']12345["\']', '"12345\'').group(0) => '"12345\'' But, well, that's OK. https://codereview.chromium.org/536553003/diff/40001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:1455: return False Refactoring suggestion: Everything in between line 1425 (reading from the file) and line 1456 (writing back to the file) can be extracted out to a helper function which just takes an original deps file contents and returns modified contents. This method could be called something like _UpdateDepsContents, and take a deps_contents, revision, and depot. Then, it would be relatively easy to add a test for just this helper method.
https://codereview.chromium.org/536553003/diff/40001/tools/bisect-perf-regres... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/536553003/diff/40001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:1443: r'(?<=[",\']%s[",\']: [",\'])([a-fA-F0-9]{40})(?=[",\'])' % On 2014/09/03 01:40:36, qyearsley wrote: > 1. The character class [",\'] in this regex matches commas as well: > > =>>> re.match(r'[",\']12345[",\']', ',12345,').group(0) > => ',12345,' > > 2. The lookahead (?=...) and lookbehind (?<=...) assertions aren't necessary; as > long as the main part that we want to capture is in a group, then we can capture > it. > > =>>> re.match(r'["\']([0-9]{5})["\']', '"12345"').group(1) > => '12345' > > So, I think the regex maybe should be: > r'["\']%s["\']: [",\']([a-fA-F0-9]{40})["\']' % deps_var > > Also, this regex doesn't guarantee that the quotes match. > > =>>> re.match(r'["\']12345["\']', '"12345\'').group(0) > => '"12345\'' > > But, well, that's OK. Done. https://codereview.chromium.org/536553003/diff/40001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:1455: return False On 2014/09/03 01:40:36, qyearsley wrote: > Refactoring suggestion: > > Everything in between line 1425 (reading from the file) and line 1456 (writing > back to the file) can be extracted out to a helper function which just takes an > original deps file contents and returns modified contents. This method could be > called something like _UpdateDepsContents, and take a deps_contents, revision, > and depot. > > Then, it would be relatively easy to add a test for just this helper method. Done.
https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... File tools/bisect-perf-regression_test.py (right): https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... tools/bisect-perf-regression_test.py:289: source_control, bisect_options) In the unit test above maybe there should be a method for creating an instance of BisectPerformanceMetrics? In a test method like this one, the details of the BisectPerformanceMetrics object aren't important, we only just want to call a method from it. https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... tools/bisect-perf-regression_test.py:293: deps_contents = bisect_perf_module.ReadStringFromFile(deps_file) Are we reading from the "src/DEPS" file here? I think it would be better to just take a few lines from the DEPS file and put it in a literal string here, so that we're not not depending on reading from the filesystem. https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... tools/bisect-perf-regression_test.py:296: git_revision = 'abcdefghijklmnopqrstuvwxyz12347890ABCD' This doesn't match the actual pattern of a SHA1 hash, since it contains letters g-z. Is this intentional?
https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... File tools/bisect-perf-regression_test.py (right): https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... tools/bisect-perf-regression_test.py:289: source_control, bisect_options) On 2014/09/03 22:18:19, qyearsley wrote: > In the unit test above maybe there should be a method for creating an instance > of BisectPerformanceMetrics? In a test method like this one, the details of the > BisectPerformanceMetrics object aren't important, we only just want to call a > method from it. Done. https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... tools/bisect-perf-regression_test.py:293: deps_contents = bisect_perf_module.ReadStringFromFile(deps_file) Yes, we are reading from src/DEPS. I added this intentionally to read from DEPS so that if anything changes in DEPs, we'll know. On 2014/09/03 22:18:19, qyearsley wrote: > Are we reading from the "src/DEPS" file here? > > I think it would be better to just take a few lines from the DEPS file and put > it in a literal string here, so that we're not not depending on reading from the > filesystem. https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... tools/bisect-perf-regression_test.py:296: git_revision = 'abcdefghijklmnopqrstuvwxyz12347890ABCD' Yes, this is intentional to check if we actually whether the DEPS contents are updated accordingly. On 2014/09/03 22:18:20, qyearsley wrote: > This doesn't match the actual pattern of a SHA1 hash, since it contains letters > g-z. Is this intentional?
https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... File tools/bisect-perf-regression_test.py (right): https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... tools/bisect-perf-regression_test.py:275: """Updates DEPS file content.""" If we have a docstring here (which I guess is unncessary), it should describe the purpose of this method -- which I think is to test that UpdateDepsContent takes a string and replaces a certain substring with another string. https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... tools/bisect-perf-regression_test.py:293: deps_contents = bisect_perf_module.ReadStringFromFile(deps_file) On 2014/09/03 22:47:38, prasadv wrote: > Yes, we are reading from src/DEPS. > I added this intentionally to read from DEPS so that if anything changes in > DEPs, we'll know. > > On 2014/09/03 22:18:19, qyearsley wrote: > > Are we reading from the "src/DEPS" file here? > > > > I think it would be better to just take a few lines from the DEPS file and put > > it in a literal string here, so that we're not not depending on reading from > the > > filesystem. > That makes sense, although that means this test method is testing something outside of the scope of just the UpdateDepsContent method. Suggestions: Either add a comment noting why we're reading from a file, or (possibly theoretically better) add a separate test method to check that the DEPS file looks how we think it does. https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... tools/bisect-perf-regression_test.py:296: git_revision = 'abcdefghijklmnopqrstuvwxyz12347890ABCD' On 2014/09/03 22:47:38, prasadv wrote: > Yes, this is intentional to check if we actually whether the DEPS contents are > updated accordingly. > On 2014/09/03 22:18:20, qyearsley wrote: > > This doesn't match the actual pattern of a SHA1 hash, since it contains > letters > > g-z. Is this intentional? > Okay, although it might be a bit confusing that it's called "git revision" but it's not a real SHA1 hash (since it contains letters other than a-f)... could change to something else like "a12345789a23456789a123456789a123456789" or make it clear that it's arbitrary, for example: replacement_string = 'foo' updated_content = bisect_instance.UpdateDepsContents( deps_contents, depot, replacement_string, deps_key)
https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... File tools/bisect-perf-regression_test.py (right): https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... tools/bisect-perf-regression_test.py:275: """Updates DEPS file content.""" Removed doc string:) On 2014/09/03 23:08:18, qyearsley wrote: > If we have a docstring here (which I guess is unncessary), it should describe > the purpose of this method -- which I think is to test that UpdateDepsContent > takes a string and replaces a certain substring with another string. https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... tools/bisect-perf-regression_test.py:293: deps_contents = bisect_perf_module.ReadStringFromFile(deps_file) On 2014/09/03 23:08:18, qyearsley wrote: > On 2014/09/03 22:47:38, prasadv wrote: > > Yes, we are reading from src/DEPS. > > I added this intentionally to read from DEPS so that if anything changes in > > DEPs, we'll know. > > > > On 2014/09/03 22:18:19, qyearsley wrote: > > > Are we reading from the "src/DEPS" file here? > > > > > > I think it would be better to just take a few lines from the DEPS file and > put > > > it in a literal string here, so that we're not not depending on reading from > > the > > > filesystem. > > > > That makes sense, although that means this test method is testing something > outside of the scope of just the UpdateDepsContent method. > > Suggestions: Either add a comment noting why we're reading from a file, or > (possibly theoretically better) add a separate test method to check that the > DEPS file looks how we think it does. Done. https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... tools/bisect-perf-regression_test.py:296: git_revision = 'abcdefghijklmnopqrstuvwxyz12347890ABCD' On 2014/09/03 23:08:18, qyearsley wrote: > On 2014/09/03 22:47:38, prasadv wrote: > > Yes, this is intentional to check if we actually whether the DEPS contents are > > updated accordingly. > > On 2014/09/03 22:18:20, qyearsley wrote: > > > This doesn't match the actual pattern of a SHA1 hash, since it contains > > letters > > > g-z. Is this intentional? > > > > Okay, although it might be a bit confusing that it's called "git revision" but > it's not a real SHA1 hash (since it contains letters other than a-f)... could > change to something else like > "a12345789a23456789a123456789a123456789" > > or make it clear that it's arbitrary, for example: > > replacement_string = 'foo' > updated_content = bisect_instance.UpdateDepsContents( > deps_contents, depot, replacement_string, deps_key) Done.
On 2014/09/03 23:22:42, prasadv wrote: > https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... > File tools/bisect-perf-regression_test.py (right): > > https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... > tools/bisect-perf-regression_test.py:275: """Updates DEPS file content.""" > Removed doc string:) > On 2014/09/03 23:08:18, qyearsley wrote: > > If we have a docstring here (which I guess is unncessary), it should describe > > the purpose of this method -- which I think is to test that UpdateDepsContent > > takes a string and replaces a certain substring with another string. > > https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... > tools/bisect-perf-regression_test.py:293: deps_contents = > bisect_perf_module.ReadStringFromFile(deps_file) > On 2014/09/03 23:08:18, qyearsley wrote: > > On 2014/09/03 22:47:38, prasadv wrote: > > > Yes, we are reading from src/DEPS. > > > I added this intentionally to read from DEPS so that if anything changes in > > > DEPs, we'll know. > > > > > > On 2014/09/03 22:18:19, qyearsley wrote: > > > > Are we reading from the "src/DEPS" file here? > > > > > > > > I think it would be better to just take a few lines from the DEPS file and > > put > > > > it in a literal string here, so that we're not not depending on reading > from > > > the > > > > filesystem. > > > > > > > That makes sense, although that means this test method is testing something > > outside of the scope of just the UpdateDepsContent method. > > > > Suggestions: Either add a comment noting why we're reading from a file, or > > (possibly theoretically better) add a separate test method to check that the > > DEPS file looks how we think it does. > > Done. > > https://codereview.chromium.org/536553003/diff/60001/tools/bisect-perf-regres... > tools/bisect-perf-regression_test.py:296: git_revision = > 'abcdefghijklmnopqrstuvwxyz12347890ABCD' > On 2014/09/03 23:08:18, qyearsley wrote: > > On 2014/09/03 22:47:38, prasadv wrote: > > > Yes, this is intentional to check if we actually whether the DEPS contents > are > > > updated accordingly. > > > On 2014/09/03 22:18:20, qyearsley wrote: > > > > This doesn't match the actual pattern of a SHA1 hash, since it contains > > > letters > > > > g-z. Is this intentional? > > > > > > > Okay, although it might be a bit confusing that it's called "git revision" but > > it's not a real SHA1 hash (since it contains letters other than a-f)... could > > change to something else like > > "a12345789a23456789a123456789a123456789" > > > > or make it clear that it's arbitrary, for example: > > > > replacement_string = 'foo' > > updated_content = bisect_instance.UpdateDepsContents( > > deps_contents, depot, replacement_string, deps_key) > > Done. PTAL...
LGTM with a couple more small nits :-) https://codereview.chromium.org/536553003/diff/100001/tools/bisect-perf-regre... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/536553003/diff/100001/tools/bisect-perf-regre... tools/bisect-perf-regression.py:1393: """Updates DEPS file with new revision of dependency repository. Note, this method doesn't actually update any files... It just returns a modified version of deps_contents. https://codereview.chromium.org/536553003/diff/100001/tools/bisect-perf-regre... File tools/bisect-perf-regression_test.py (right): https://codereview.chromium.org/536553003/diff/100001/tools/bisect-perf-regre... tools/bisect-perf-regression_test.py:15: """Returns an instance of BisectPerformanceMetrics class.""" Formatting nit: extra space. (And maybe it should be "an instance of the BisectPerformanceMetrics class.") https://codereview.chromium.org/536553003/diff/100001/tools/bisect-perf-regre... tools/bisect-perf-regression_test.py:32: return bisect_instance Formatting nit: Two blank spaces between top-level code blocks. (Also, this could be marked with a _ to show that we don't want to use it in other modules, but this isn't important here.) https://codereview.chromium.org/536553003/diff/100001/tools/bisect-perf-regre... tools/bisect-perf-regression_test.py:287: Formatting nit: should be 2 blank lines rather than 3.
final pass please:) https://codereview.chromium.org/536553003/diff/100001/tools/bisect-perf-regre... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/536553003/diff/100001/tools/bisect-perf-regre... tools/bisect-perf-regression.py:1393: """Updates DEPS file with new revision of dependency repository. On 2014/09/04 18:42:35, qyearsley wrote: > Note, this method doesn't actually update any files... It just returns a > modified version of deps_contents. Done. https://codereview.chromium.org/536553003/diff/100001/tools/bisect-perf-regre... File tools/bisect-perf-regression_test.py (right): https://codereview.chromium.org/536553003/diff/100001/tools/bisect-perf-regre... tools/bisect-perf-regression_test.py:15: """Returns an instance of BisectPerformanceMetrics class.""" On 2014/09/04 18:42:36, qyearsley wrote: > Formatting nit: extra space. (And maybe it should be "an instance of the > BisectPerformanceMetrics class.") Done. https://codereview.chromium.org/536553003/diff/100001/tools/bisect-perf-regre... tools/bisect-perf-regression_test.py:32: return bisect_instance On 2014/09/04 18:42:36, qyearsley wrote: > Formatting nit: Two blank spaces between top-level code blocks. > (Also, this could be marked with a _ to show that we don't want to use it in > other modules, but this isn't important here.) Done. https://codereview.chromium.org/536553003/diff/100001/tools/bisect-perf-regre... tools/bisect-perf-regression_test.py:287: On 2014/09/04 18:42:36, qyearsley wrote: > Formatting nit: should be 2 blank lines rather than 3. Done.
The CQ bit was checked by prasadv@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/prasadv@chromium.org/536553003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as 683ed42c365323255a89bed039fccb01b5c792c8
Message was sent while issue was closed.
LGTM to commit now. There's still potential for improvement though, I think. https://codereview.chromium.org/536553003/diff/120001/tools/bisect-perf-regre... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/536553003/diff/120001/tools/bisect-perf-regre... tools/bisect-perf-regression.py:1408: new_data = None "data" is a pretty non-descriptive word to use in general, and often there are better more specific words, such as "text" or "contents". https://codereview.chromium.org/536553003/diff/120001/tools/bisect-perf-regre... tools/bisect-perf-regression.py:1425: if new_data: This could be changed to if new_data and depot == 'v8_bleeding_edge':
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/87fdd0ac5c9604e2ef3af7cd845ac254b7dd51b8 Cr-Commit-Position: refs/heads/master@{#293351} |
