launchpad-reviewers team mailing list archive

[Colin Watson <mp+421848@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/lpbuildbot/remove-testfix-mode into lp:lpbuildbot.

Commit message:
Remove testfix mode.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/lpbuildbot/remove-testfix-mode/+merge/421848

The Launchpad buildbot has historically had a "testfix" mode where it doesn't re-run tests automatically if the previous run failed, and either has to be forced or has to see a change with "[testfix]" in its commit message.  This was originally intended to arrange that if somebody broke tests then we wouldn't dispatch long test runs until they were believed to be fixed.  However, in recent years this has been much more often annoying than helpful: we have a number of flaky tests, and our landing velocity isn't usually such that a spurious test run after a previous failure would be the limiting factor (and our workers are devops now so we can always interrupt doomed runs manually if need be).

The aggregating scheduler now serves only to batch up multiple changes that arrive while a test run is still in progress, without the additional testfix mode.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/lpbuildbot/remove-testfix-mode into lp:lpbuildbot.

=== modified file 'lpbuildbot/schedulers/aggregating.py'
--- lpbuildbot/schedulers/aggregating.py	2021-06-11 11:45:05 +0000
+++ lpbuildbot/schedulers/aggregating.py	2022-05-10 12:49:15 +0000
@@ -10,17 +10,8 @@
 from buildbot.status.results import SUCCESS
 
 
-def is_testfix(change):
-    return change.comments and '[testfix]' in change.comments.lower()
-
-
 class AggregatingScheduler(SingleBranchScheduler):
     # states:
-    # - failure ("[testfix]"):
-    #   * we have a change with "[testfix]" in the message: run build
-    #     immediately.
-    #   * we do not have a "[testfix]" change: do nothing.
-    # - non-failure:
     #   * treeStableTimer is None; or treeStableCount is set and change count
     #     >= treeStableCount; or treeStableTimer is set, and time since last
     #     change >= treeStableTimer; or timer has fired: run tests immediately
@@ -34,7 +25,7 @@
         SingleBranchScheduler.__init__(
             self, name, builderNames=builderNames, properties=properties,
             change_filter=change_filter, branch=branch,
-            treeStableTimer=treeStableTimer, fileIsImportant=is_testfix)
+            treeStableTimer=treeStableTimer)
 
         if treeStableTimer is None and treeStableCount is not None:
             raise ValueError(
@@ -45,19 +36,6 @@
         self._branch = branch
         self._changes = []
         self._nextBuildTime = None
-        self._failing = False
-
-    def startService(self):
-        for name in self.builderNames:
-            builder = self.master.status.getBuilder(name)
-            last_build = builder.getLastFinishedBuild()
-            if (not self._failing and last_build is not None and
-                    last_build.getResults() != SUCCESS):
-                # The previous build failed; we want to remember that and be
-                # in [testfix] mode.
-                self._failing = True
-                log.msg('%s remembering previous failure' % (self,))
-        SingleBranchScheduler.startService(self)
 
     def getPendingBuildTimes(self):
         if self._nextBuildTime is not None:
@@ -113,40 +91,28 @@
             del self._stable_timers[timer_name]
 
         build_now = False
-        if self._failing:
-            self._nextBuildTime = None
-            if any(classifications.values()):
-                log.msg('%s scheduling testfix build immediately' % (self,))
-                yield self.scheduleBuild()
-            else:
-                # In failing mode, and no testfix.  Will not schedule.
-                log.msg(
-                    '%s in failing mode and no "[testfix]" in comment, '
-                    'so no build' % (self,))
-        else:
-            build_now = False
-            if self.treeStableTimer is not None:
-                changeids = sorted(classifications)
-                chdict = yield self.master.db.changes.getChange(changeids[-1])
-                change = yield Change.fromChdict(self.master, chdict)
-                self._nextBuildTime = max(
-                    self._nextBuildTime, change.when + self.treeStableTimer)
-                if self.treeStableCount:
-                    if len(classifications) >= self.treeStableCount:
-                        build_now = True
-            else:
-                build_now = True
-            if build_now:
-                log.msg('%s scheduling build immediately' % (self,))
-                yield self.scheduleBuild()
-            else:
-                log.msg('%s scheduling build in %s seconds' %
-                        (self, self.treeStableTimer))
-                def fire_timer():
-                    d = self.stableTimerFired()
-                    d.addErrback(log.err, 'while firing stable timer')
-                self._stable_timers[timer_name] = self._reactor.callLater(
-                    self.treeStableTimer, fire_timer)
+        if self.treeStableTimer is not None:
+            changeids = sorted(classifications)
+            chdict = yield self.master.db.changes.getChange(changeids[-1])
+            change = yield Change.fromChdict(self.master, chdict)
+            self._nextBuildTime = max(
+                self._nextBuildTime, change.when + self.treeStableTimer)
+            if self.treeStableCount:
+                if len(classifications) >= self.treeStableCount:
+                    build_now = True
+        else:
+            build_now = True
+        if build_now:
+            log.msg('%s scheduling build immediately' % (self,))
+            yield self.scheduleBuild()
+        else:
+            log.msg('%s scheduling build in %s seconds' %
+                    (self, self.treeStableTimer))
+            def fire_timer():
+                d = self.stableTimerFired()
+                d.addErrback(log.err, 'while firing stable timer')
+            self._stable_timers[timer_name] = self._reactor.callLater(
+                self.treeStableTimer, fire_timer)
 
     @defer.inlineCallbacks
     def scheduleBuild(self, reason='scheduler', builderNames=None):
@@ -180,8 +146,7 @@
 
     @defer.inlineCallbacks
     def _buildsetFinished(self, bss):
-        self._failing = bss.getResults() != SUCCESS
-        if self._failing:
+        if bss.getResults() != SUCCESS:
             log.msg('%s last build did not succeed (%r)' % (
                 self, bss.getResults()))
         yield self._maybeAddBuildset()