From 05c9a5f7b87cabfecc81d8d2744abe1128a92b00 Mon Sep 17 00:00:00 2001
From: Joseph Schorr <josephschorr@users.noreply.github.com>
Date: Tue, 22 Sep 2015 14:44:49 -0400
Subject: [PATCH] Fix the skip branch logic

---
 buildtrigger/triggerutil.py | 18 ++++++-------
 test/test_trigger.py        | 54 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 62 insertions(+), 10 deletions(-)

diff --git a/buildtrigger/triggerutil.py b/buildtrigger/triggerutil.py
index 3786dd7d4..40c1a7bde 100644
--- a/buildtrigger/triggerutil.py
+++ b/buildtrigger/triggerutil.py
@@ -75,28 +75,28 @@ def should_skip_commit(message):
   return '[skip build]' in message or '[build skip]' in message
 
 
-def raise_if_skipped_build(prepared_build):
+def raise_if_skipped_build(prepared_build, config):
   """ Raises a SkipRequestException if the given build should be skipped. """
+  # Check to ensure we have metadata.
   if not prepared_build.metadata:
     logger.debug('Skipping request due to missing metadata for prepared build')
     raise SkipRequestException()
 
-  if should_skip_commit(prepared_build.metadata['commit_info']['message']):
-    logger.debug('Skipping request due to commit message request')
-    raise SkipRequestException()
-
-
-def raise_if_skipped(config, ref):
-  """ Raises a SkipRequestException if the given ref should be skipped. """
+  # Check the branchtag regex.
   if 'branchtag_regex' in config:
     try:
       regex = re.compile(config['branchtag_regex'])
     except:
       regex = re.compile('.*')
 
-    if not matches_ref(ref, regex):
+    if not matches_ref(prepared_build.metadata.get('ref'), regex):
       raise SkipRequestException()
 
+  # Check the commit message.
+  if should_skip_commit(prepared_build.metadata['commit_info']['message']):
+    logger.debug('Skipping request due to commit message request')
+    raise SkipRequestException()
+
 
 def matches_ref(ref, regex):
   match_string = ref.split('/', 1)[1]
diff --git a/test/test_trigger.py b/test/test_trigger.py
index 710dab20b..8cdb8921a 100644
--- a/test/test_trigger.py
+++ b/test/test_trigger.py
@@ -1,7 +1,9 @@
 import unittest
 import re
 
-from buildtrigger.triggerutil import matches_ref
+from buildtrigger.triggerutil import matches_ref, raise_if_skipped_build
+from buildtrigger.triggerutil import SkipRequestException
+from endpoints.building import PreparedBuild
 
 class TestRegex(unittest.TestCase):
   def assertDoesNotMatch(self, ref, filt):
@@ -25,5 +27,55 @@ class TestRegex(unittest.TestCase):
 
     self.assertDoesNotMatch('ref/heads/delta', '(((heads/alpha)|(heads/beta))|(heads/gamma))|(heads/master)')
 
+
+class TestSkipBuild(unittest.TestCase):
+  def testSkipNoMetadata(self):
+    prepared = PreparedBuild()
+    prepared.metadata = {}
+    config = {}
+
+    self.assertRaises(SkipRequestException, raise_if_skipped_build, prepared, config)
+
+  def testSkipByBranchtagRegex(self):
+    prepared = PreparedBuild()
+    prepared.metadata = {
+      'ref': 'ref/heads/master',
+    }
+
+    config = {
+      'branchtag_regex': 'nothing'
+    }
+    self.assertRaises(SkipRequestException, raise_if_skipped_build, prepared, config)
+
+  def testSkipByMessage(self):
+    prepared = PreparedBuild()
+    prepared.metadata = {
+      'ref': 'ref/heads/master',
+      'commit_info': {
+        'message': '[skip build]',
+      },
+    }
+
+    config = {}
+
+    self.assertRaises(SkipRequestException, raise_if_skipped_build, prepared, config)
+
+
+  def testDoesNotSkip(self):
+    prepared = PreparedBuild()
+    prepared.metadata = {
+      'ref': 'ref/heads/master',
+      'commit_info': {
+        'message': 'some cool message',
+      },
+    }
+
+    config = {
+      'branchtag_regex': '(master)|(heads/master)',
+    }
+
+    raise_if_skipped_build(prepared, config)
+
+
 if __name__ == '__main__':
   unittest.main()