Ensure PRs arent rejected for stale negative reviews
This updates the github source to only use the most recent review
from each user reviewing a PR. This avoids having obsolete negative
reviews trump a newer positive review.
I've added a new test for good measure to ensure it works for the
github case, where we are doing some conversion of github formatted
timestamps to internal timestamps.
Change-Id: I5607901def856c9363ec786a4116bfec19c9c97c
Co-Authored-By: Jesse Keating <omgjlk@us.ibm.com>
Signed-off-by: Adam Gandelman <adamg@ubuntu.com>
diff --git a/tests/base.py b/tests/base.py
index 9232d52..71c48aa 100755
--- a/tests/base.py
+++ b/tests/base.py
@@ -16,6 +16,7 @@
# under the License.
from six.moves import configparser as ConfigParser
+import datetime
import gc
import hashlib
import json
@@ -777,23 +778,37 @@
}))
def addReview(self, user, state, granted_on=None):
- # Each user will only have one review at a time, so replace
- # any existing reviews
- # FIXME(jlk): this isn't quite right, reviews stack, we only
- # consider the latest for a user. Thanks GitHub!!
- for review in self.reviews:
- if review['user']['login'] == user:
- self.reviews.remove(review)
+ gh_time_format = '%Y-%m-%dT%H:%M:%SZ'
+ # convert the timestamp to a str format that would be returned
+ # from github as 'submitted_at' in the API response
- if not granted_on:
- granted_on = time.time()
+ if granted_on:
+ granted_on = datetime.datetime.utcfromtimestamp(granted_on)
+ submitted_at = time.strftime(
+ gh_time_format, granted_on.timetuple())
+ else:
+ # github timestamps only down to the second, so we need to make
+ # sure reviews that tests add appear to be added over a period of
+ # time in the past and not all at once.
+ if not self.reviews:
+ # the first review happens 10 mins ago
+ offset = 600
+ else:
+ # subsequent reviews happen 1 minute closer to now
+ offset = 600 - (len(self.reviews) * 60)
+
+ granted_on = datetime.datetime.utcfromtimestamp(
+ time.time() - offset)
+ submitted_at = time.strftime(
+ gh_time_format, granted_on.timetuple())
+
self.reviews.append({
'state': state,
'user': {
'login': user,
'email': user + "@derp.com",
},
- 'provided': int(granted_on),
+ 'submitted_at': submitted_at,
})
def _getPRReference(self):
diff --git a/tests/fixtures/layouts/requirements-github.yaml b/tests/fixtures/layouts/requirements-github.yaml
index 6fc94c2..5b92b58 100644
--- a/tests/fixtures/layouts/requirements-github.yaml
+++ b/tests/fixtures/layouts/requirements-github.yaml
@@ -53,6 +53,11 @@
review:
- type: approved
permission: write
+ reject:
+ github:
+ review:
+ - type: changes_requested
+ permission: write
trigger:
github:
- event: pull_request
@@ -71,6 +76,47 @@
- username: 'derp'
type: approved
permission: write
+ reject:
+ github:
+ review:
+ - type: changes_requested
+ permission: write
+ trigger:
+ github:
+ - event: pull_request
+ action: comment
+ comment: 'test me'
+ success:
+ github:
+ comment: true
+
+- pipeline:
+ name: newer_than
+ manager: independent
+ require:
+ github:
+ review:
+ - type: approved
+ permission: write
+ newer-than: 1d
+ trigger:
+ github:
+ - event: pull_request
+ action: comment
+ comment: 'test me'
+ success:
+ github:
+ comment: true
+
+- pipeline:
+ name: older_than
+ manager: independent
+ require:
+ github:
+ review:
+ - type: approved
+ permission: write
+ older-than: 1d
trigger:
github:
- event: pull_request
@@ -90,6 +136,10 @@
name: project4-reviewreq
- job:
name: project5-reviewuserstate
+- job:
+ name: project6-newerthan
+- job:
+ name: project7-olderthan
- project:
name: org/project1
@@ -120,3 +170,15 @@
reviewuserstate:
jobs:
- project5-reviewuserstate
+
+- project:
+ name: org/project6
+ newer_than:
+ jobs:
+ - project6-newerthan
+
+- project:
+ name: org/project7
+ older_than:
+ jobs:
+ - project7-olderthan
diff --git a/tests/unit/test_github_requirements.py b/tests/unit/test_github_requirements.py
index 3f69307..60bcf74 100644
--- a/tests/unit/test_github_requirements.py
+++ b/tests/unit/test_github_requirements.py
@@ -13,6 +13,8 @@
# License for the specific language governing permissions and limitations
# under the License.
+import time
+
from tests.base import ZuulTestCase, simple_layout
@@ -171,3 +173,94 @@
self.waitUntilSettled()
self.assertEqual(len(self.history), 1)
self.assertEqual(self.history[0].name, 'project5-reviewuserstate')
+
+# TODO: Implement reject on approval username/state
+
+ @simple_layout('layouts/requirements-github.yaml', driver='github')
+ def test_pipeline_require_review_latest_user_state(self):
+ "Test pipeline requirement: review state from user"
+
+ A = self.fake_github.openFakePullRequest('org/project5', 'master', 'A')
+ # Add derp and herp to writers
+ A.writers.extend(('derp', 'herp'))
+ # A comment event that we will keep submitting to trigger
+ comment = A.getCommentAddedEvent('test me')
+ self.fake_github.emitEvent(comment)
+ self.waitUntilSettled()
+ # No positive review from derp so should not be enqueued
+ self.assertEqual(len(self.history), 0)
+
+ # The first negative review from derp should not cause it to be
+ # enqueued
+ for i in range(1, 4):
+ submitted_at = time.time() - 72 * 60 * 60
+ A.addReview('derp', 'CHANGES_REQUESTED',
+ submitted_at)
+ self.fake_github.emitEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 0)
+
+ # A positive review from derp should cause it to be enqueued
+ A.addReview('derp', 'APPROVED')
+ self.fake_github.emitEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, 'project5-reviewuserstate')
+
+ @simple_layout('layouts/requirements-github.yaml', driver='github')
+ def test_require_review_newer_than(self):
+
+ A = self.fake_github.openFakePullRequest('org/project6', 'master', 'A')
+ # Add derp and herp to writers
+ A.writers.extend(('derp', 'herp'))
+ # A comment event that we will keep submitting to trigger
+ comment = A.getCommentAddedEvent('test me')
+ self.fake_github.emitEvent(comment)
+ self.waitUntilSettled()
+ # No positive review from derp so should not be enqueued
+ self.assertEqual(len(self.history), 0)
+
+ # Add a too-old positive review, should not be enqueued
+ submitted_at = time.time() - 72 * 60 * 60
+ A.addReview('derp', 'APPROVED',
+ submitted_at)
+ self.fake_github.emitEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 0)
+
+ # Add a recent positive review
+ submitted_at = time.time() - 12 * 60 * 60
+ A.addReview('derp', 'APPROVED', submitted_at)
+ self.fake_github.emitEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, 'project6-newerthan')
+
+ @simple_layout('layouts/requirements-github.yaml', driver='github')
+ def test_require_review_older_than(self):
+
+ A = self.fake_github.openFakePullRequest('org/project7', 'master', 'A')
+ # Add derp and herp to writers
+ A.writers.extend(('derp', 'herp'))
+ # A comment event that we will keep submitting to trigger
+ comment = A.getCommentAddedEvent('test me')
+ self.fake_github.emitEvent(comment)
+ self.waitUntilSettled()
+ # No positive review from derp so should not be enqueued
+ self.assertEqual(len(self.history), 0)
+
+ # Add a too-new positive, should not be enqueued
+ submitted_at = time.time() - 12 * 60 * 60
+ A.addReview('derp', 'APPROVED',
+ submitted_at)
+ self.fake_github.emitEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 0)
+
+ # Add an old enough positive, should enqueue
+ submitted_at = time.time() - 72 * 60 * 60
+ A.addReview('herp', 'APPROVED', submitted_at)
+ self.fake_github.emitEvent(comment)
+ self.waitUntilSettled()
+ self.assertEqual(len(self.history), 1)
+ self.assertEqual(self.history[0].name, 'project7-olderthan')