Parse Git hash for dependency repositories from DEPS file.

tools/bisect-perf-regression_test.py

 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import os
+import re
 import unittest
 
 from auto_bisect import source_control as source_control_module
 
 # Special import necessary because filename contains dash characters.
 bisect_perf_module = __import__('bisect-perf-regression')
 
 
 class BisectPerfRegressionTest(unittest.TestCase):
   """Test case for other functions and classes in bisect-perf-regression.py."""
@@ skipping 247 matching lines @@
     bisect_options = bisect_perf_module.BisectOptions.FromDict(options_dict)
     source_control = source_control_module.DetermineAndCreateSourceControl(
         bisect_options)
 
     cp_git_rev = '7017a81991de983e12ab50dfc071c70e06979531'
     self.assertEqual(291765, source_control.SVNFindRev(cp_git_rev))
 
     svn_git_rev = 'e6db23a037cad47299a94b155b95eebd1ee61a58'
     self.assertEqual(291467, source_control.SVNFindRev(svn_git_rev))
 
+  def testUpdateDepsContent(self):
+    """Updates DEPS file content."""

[qyearsley, 2014/09/03 23:08:18]

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.

[prasadv, 2014/09/03 23:22:41]

Removed doc string:)
On 2014/09/03 23:08:18, qyearsley wrote:

+    options_dict = {
+      'debug_ignore_build': True,
+      'debug_ignore_sync': True,
+      'debug_ignore_perf_test': True,
+      'command': 'fake_command',
+      'metric': 'fake/metric',
+      'good_revision': 280000,
+      'bad_revision': 280005,
+    }
+    bisect_options = bisect_perf_module.BisectOptions.FromDict(options_dict)
+    source_control = source_control_module.DetermineAndCreateSourceControl(
+        bisect_options)
+    bisect_instance = bisect_perf_module.BisectPerformanceMetrics(
+        source_control, bisect_options)

[qyearsley, 2014/09/03 22:18:19]

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.

[prasadv, 2014/09/03 22:47:38]

Done.

+    bisect_instance.src_cwd = os.path.abspath(
+        os.path.join(os.path.dirname(__file__), os.path.pardir))
+    deps_file = 'DEPS'
+    deps_contents = bisect_perf_module.ReadStringFromFile(deps_file)

[qyearsley, 2014/09/03 22:18:19]

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.

[prasadv, 2014/09/03 22:47:38]

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:

[qyearsley, 2014/09/03 23:08:18]

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.

[prasadv, 2014/09/03 23:22:41]

Done.

+    deps_key = 'v8_revision'
+    depot = 'v8'
+    git_revision = 'abcdefghijklmnopqrstuvwxyz12347890ABCD'

[qyearsley, 2014/09/03 22:18:20]

This doesn't match the actual pattern of a SHA1 hash, since it contains letters
g-z. Is this intentional?

[prasadv, 2014/09/03 22:47:38]

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:

[qyearsley, 2014/09/03 23:08:18]

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)

[prasadv, 2014/09/03 23:22:41]

Done.

+    updated_content = bisect_instance.UpdateDepsContents(
+        deps_contents, depot, git_revision, deps_key)
+    self.assertIsNotNone(updated_content)
+    ss = re.compile('["\']%s["\']: ["\']%s["\']' % (deps_key, git_revision))
+    self.assertIsNotNone(re.search(ss, updated_content))
+
+
+
 if __name__ == '__main__':
   unittest.main()