On Mon, Mar 5, 2012 at 2:27 AM, Keshav Kini <keshav.k...@gmail.com> wrote: > Robert Bradshaw <rober...@math.washington.edu> writes: >> On Sun, Mar 4, 2012 at 5:26 AM, Keshav Kini <keshav.k...@gmail.com> wrote: >>> Robert Bradshaw <rober...@math.washington.edu> writes: >>>> On Fri, Mar 2, 2012 at 5:35 AM, Keshav Kini <keshav.k...@gmail.com> wrote: >>>>> I would actually like the patchbot to NOT do continuous builds and >>>>> tests, once we move to a push/pull system. There should be a big button >>>>> on each ticket which says "Test Me!", which will cause the patchbot to >>>>> fetch whatever branch is specified on the ticket (in some "branch" >>>>> field) and put the pair "(ticket, commit-id)" into a queue. (The >>>>> patchbot wouldn't actually test commit-id, but would attempt to >>>>> temporarily merge commit-id into master, and then test the resulting >>>>> commit or return a failure to automatically merge / report that the >>>>> branch needed merging / rebasing.) >>>> >>>> I think that each patchbot could, according to its configuration, >>>> periodically pull a newer version of master (and run tests against >>>> that to isolate faults before trying to apply tickets). This is >>>> essentially what we do now, but "master" is whatever sage version (or >>>> alpha) you're running and doesn't change. (This also bleeds into the >>>> discussion about being able to upgrade sage and all its dependencies >>>> by syncing a single repository...) >>>> >>>>> One reason for this: once we switch to a push/pull system, commits will >>>>> hopefully be a lot more frequent, and I question whether we would even >>>>> want the patchbot to test each and every commit over the whole doctest >>>>> suite. >>>> >>>> Setting a ticket to "needs review" is basically this button, I don't >>>> think we need a separate one. >>> >>> The difference is that tickets do not automatically get set back to >>> "doesn't need review" when you push new commits, >> >> Nor should it, if this is still the correct state (which it often is). > > Of course, but... well, see below. > >>> nor does the patchbot >>> get notified, necessarily, when you push new commits. >> >> But there's no reason it couldn't. >> >>> This should fundamentally be a "request action" type of thing, not "set >>> status" type >>> of thing, IMO. >> >> Can you think of any case where you would want to change the status to >> "needs review" and not activate the patchbot? > > No, but I *can* think of cases where I would want to activate the > patchbot and not change the status to "needs review" - for example, at > some of the times when I have pushed new commits to a branch which I've > already marked as "needs review". This will no doubt be a common > practice, just as it is common to push more commits to open pull > requests in github's workflow, even if nobody has indicated they don't > like the current commits.
In which case the patchbot will still be triggered due to needs review + out of date tests. >>>> We wouldn't test "every commit," it's >>>> the tips of what gets pushed (or, possibly, when the master progresses >>>> forward enough). >>> >>> I don't want pushing my commits to lead to side-effects. People should >>> be encouraged to push their commits somewhere public as soon as possible >>> so people can see what they are doing and collaborate. >> >> I agree, which is fine as long as the patchbot is side effect free >> (other than providing additional information, perhaps even in an >> (easily ignored, but usually useful) email for failures). > > Well, I was considering the computation time required for a doctest > suite runthrough as a "side effect". But we truly have as much of a glut > of CPU cycles as you say... :) And again we can make these automatic > runs have lower priority anyway. > >>> I would want to avoid automatic doctests marking anything as "failing", >>> though. The current red/green/yellow status should be based on the last >>> manually-requested doctest, not the last doctest. Perhaps green and red >>> for "last manually requested doctest passed" and "last manually >>> requested doctest failed", respectively, with an overlaid question mark >>> if the branch set on the ticket is not currently on the commit which was >>> last manually tested. >> >> I can't think of any case that the results of the last manually >> requested run would be preferable to a more recent automatically >> triggered result. > > Here's what I had in mind: I push a commit. It works! The patchbot gives > its OK to my branch. I push another commit. Oops, I broke it. I realize > this soon thereafter, and begin working on a fix. Meanwhile, the > patchbot sees my broken commit and tests it. When the tests fail, my > branch is marked as broken. I finally finish fixing the problem, and > push my fix, but now the patchbot happens to be overloaded and my new > branch head is stuck at the end of a long testing queue. My branch is > continues to be marked broken. Everyone is ignoring my ticket even > though it is ready for review! Even if the ticket displays something > like "test result out of date", a good test result that's out of date > inspires more confidence than a bad test result that's out of date, and > would have attracted more reviewers. Oh, how I wish I could have stopped > that automatic test of my broken commit! > > ... OK, I can see my scenario is not exactly a big deal, now that I > write it out like that... :). In this case you quickly fire up/tweek your own patchbot client with an absurdity high priority to test that one ticket. In any case, better than a good test result on a bad branch. > By the way, if tickets instead just display "pending" whenever their > tests are out of date, without disclosing what the latest (now > out-of-date) result was, it seems possible that patchbot results will > become hidden by a "pending" mark much or most of the time, especially > if I commit faster than the patchbot can test my commits. This makes the > indicator much less useful, so I assume it will not be set up that way. Yeah, I agree it would be useful to have some kind of qualified "the last iteration of this branch passed/failed" status when it becomes out of date due to new commits. Generally peoples workflow seems to be commit, commit, commit, push, [lots of time passes], commit, commit, push [more time passes. - Robert -- To post to this group, send an email to sage-devel@googlegroups.com To unsubscribe from this group, send an email to sage-devel+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/sage-devel URL: http://www.sagemath.org
