Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-4617

"Include culprits" option not working

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Component/s: core
    • Labels:
      None
    • Environment:
      Platform: All, OS: All

      Description

      Although I have the Include culprits option checked for Unstable builds, the
      emails still only go to the global + committer, but not the culprits.

        Issue Links

          Activity

          Hide
          mcrooney mcrooney added a comment -

          To elaborate, I have an Unstable trigger sent to email Recipients, Committers,
          and Culprits. Person A is on the global list, and person B commits and breaks a
          test. The build is now unstable and an email goes to A+B. Now person C commits,
          and the test still fails because no one has fixed it yet. An email goes to A+C,
          instead of A+B+C.

          So it seems that include culprits isn't working, which is very useful to remind
          the person who broke the test that it is affecting other committers and they
          should fix it in a timely manner.

          Show
          mcrooney mcrooney added a comment - To elaborate, I have an Unstable trigger sent to email Recipients, Committers, and Culprits. Person A is on the global list, and person B commits and breaks a test. The build is now unstable and an email goes to A+B. Now person C commits, and the test still fails because no one has fixed it yet. An email goes to A+C, instead of A+B+C. So it seems that include culprits isn't working, which is very useful to remind the person who broke the test that it is affecting other committers and they should fix it in a timely manner.
          Hide
          mindless Alan Harder added a comment -

          Can you update the latest email-ext (released today) and see if this is still a problem? Also turn on Hudson's built-in email notification with "Send separate e-mails to individuals who broke the build" checked, and see if it has the same problem (this also sends to build.getCulprits()).

          Show
          mindless Alan Harder added a comment - Can you update the latest email-ext (released today) and see if this is still a problem? Also turn on Hudson's built-in email notification with "Send separate e-mails to individuals who broke the build" checked, and see if it has the same problem (this also sends to build.getCulprits()).
          Hide
          mindless Alan Harder added a comment -

          Reminder to retest on current releases, thanks..

          Show
          mindless Alan Harder added a comment - Reminder to retest on current releases, thanks..
          Hide
          mcrooney mcrooney added a comment -

          Still an issue with Hudson 1.341 and email-ext 2.4, indeed.

          Show
          mcrooney mcrooney added a comment - Still an issue with Hudson 1.341 and email-ext 2.4, indeed.
          Hide
          mindless Alan Harder added a comment -

          what SCM are you using? I wasn't able to reproduce this problem with a subversion project. In a project where email-ext isn't emailing the right people, access /job/JOBNAME/BUILDNUMBER/api/xml and look at the culprit info listed here.. is this list accurate? (email-ext should be using that list)

          Show
          mindless Alan Harder added a comment - what SCM are you using? I wasn't able to reproduce this problem with a subversion project. In a project where email-ext isn't emailing the right people, access /job/JOBNAME/BUILDNUMBER/api/xml and look at the culprit info listed here.. is this list accurate? (email-ext should be using that list)
          Hide
          mcrooney mcrooney added a comment -

          We are using SVN. Were you trying with sequential Unstable builds with different committers, not Failed builds? If I look at the api/xml it is indeed wrong there. The first unstable build it has the two committers as culprits (correct), but the next unstable build doesn't have those people but instead the two people that made commits in that revision set. It doesn't seem to be carrying over culprits.

          Show
          mcrooney mcrooney added a comment - We are using SVN. Were you trying with sequential Unstable builds with different committers, not Failed builds? If I look at the api/xml it is indeed wrong there. The first unstable build it has the two committers as culprits (correct), but the next unstable build doesn't have those people but instead the two people that made commits in that revision set. It doesn't seem to be carrying over culprits.
          Hide
          mindless Alan Harder added a comment -

          What is your Hudson version and subversion plugin version?

          Moving out of email-ext component, as core determines the culprit list and email-ext just uses that info from the build. If the remote API also has incorrect data, the problem is not in email-ext.

          I did: failed build with personA, failed build with personB, unstable build
          I'll try now with only unstables..

          Show
          mindless Alan Harder added a comment - What is your Hudson version and subversion plugin version? Moving out of email-ext component, as core determines the culprit list and email-ext just uses that info from the build. If the remote API also has incorrect data, the problem is not in email-ext. I did: failed build with personA, failed build with personB, unstable build I'll try now with only unstables..
          Hide
          mcrooney mcrooney added a comment -

          Hudson 1.341 with Subversion 1.8. I'll upgrade to 1.9 and test but I don't see anything relevant in the changes.

          Show
          mcrooney mcrooney added a comment - Hudson 1.341 with Subversion 1.8. I'll upgrade to 1.9 and test but I don't see anything relevant in the changes.
          Hide
          mindless Alan Harder added a comment -

          Yup, sequence of unstables shows the problem. Odd that unstable following a failed build does pull cuprits from that previous failed build, but doesn't pull from previous unstable.

          Show
          mindless Alan Harder added a comment - Yup, sequence of unstables shows the problem. Odd that unstable following a failed build does pull cuprits from that previous failed build, but doesn't pull from previous unstable.
          Hide
          mindless Alan Harder added a comment -

          Looks like "culprits" was coded to only be those causing failed builds (not unstable).. it has been that way since r4689 (Sep 2007). See the request in JENKINS-747.. no mention of unstable builds there.

          So, should the culprits calculated by Hudson core be since last stable build (to catch committers for unstable builds) or since last "successful" build (which may be unstable.. so only tracking failed builds, or aborted I guess).

          Show
          mindless Alan Harder added a comment - Looks like "culprits" was coded to only be those causing failed builds (not unstable).. it has been that way since r4689 (Sep 2007). See the request in JENKINS-747 .. no mention of unstable builds there. So, should the culprits calculated by Hudson core be since last stable build (to catch committers for unstable builds) or since last "successful" build (which may be unstable.. so only tracking failed builds, or aborted I guess).
          Hide
          mcrooney mcrooney added a comment -

          I guess it is obvious since I filed this bug but intuitively to me someone who broke tests is a "culprit".

          Show
          mcrooney mcrooney added a comment - I guess it is obvious since I filed this bug but intuitively to me someone who broke tests is a "culprit".
          Hide
          kohsuke Kohsuke Kawaguchi added a comment -

          I suppose it's a reasonable change to include those who broke the tests.

          The only thing I wonder is if we can do something clever in situations where you have some tests regularly failing, then people who committed a change that didn't increase the failure count wouldn't be counted toward culprits. But I suppose it's not possible since it collapses multiple abstraction layers.

          Show
          kohsuke Kohsuke Kawaguchi added a comment - I suppose it's a reasonable change to include those who broke the tests. The only thing I wonder is if we can do something clever in situations where you have some tests regularly failing, then people who committed a change that didn't increase the failure count wouldn't be counted toward culprits. But I suppose it's not possible since it collapses multiple abstraction layers.
          Hide
          mwebber mwebber added a comment -

          kohsuke commented:
          >> The only thing I wonder is if we can do something clever in situations where you have some tests regularly failing, then people who committed a change that didn't increase the failure count wouldn't be counted toward culprits

          JENKINS-5282 requests this very change.

          Show
          mwebber mwebber added a comment - kohsuke commented: >> The only thing I wonder is if we can do something clever in situations where you have some tests regularly failing, then people who committed a change that didn't increase the failure count wouldn't be counted toward culprits JENKINS-5282 requests this very change.
          Hide
          mcrooney mcrooney added a comment -

          Just as an update on this issue, I attempted to fix this locally and will commit if it works, though I'm not convinced as of yet.

          Index: core/src/main/java/hudson/model/AbstractBuild.java
          ===================================================================
          --- core/src/main/java/hudson/model/AbstractBuild.java	(revision 31234)
          +++ core/src/main/java/hudson/model/AbstractBuild.java	(working copy)
          @@ -263,7 +263,7 @@
                       R p = getPreviousCompletedBuild();
                       if (p !=null && isBuilding()) {
                           Result pr = p.getResult();
          -                if (pr!=null && pr.isWorseThan(Result.UNSTABLE)) {
          +                if (pr!=null && pr.isWorseOrEqualTo(Result.UNSTABLE)) {
                               // we are still building, so this is just the current latest information,
                               // but we seems to be failing so far, so inherit culprits from the previous build.
                               // isBuilding() check is to avoid recursion when loading data from old Hudson, which doesn't record
          
          Show
          mcrooney mcrooney added a comment - Just as an update on this issue, I attempted to fix this locally and will commit if it works, though I'm not convinced as of yet. Index: core/src/main/java/hudson/model/AbstractBuild.java =================================================================== --- core/src/main/java/hudson/model/AbstractBuild.java (revision 31234) +++ core/src/main/java/hudson/model/AbstractBuild.java (working copy) @@ -263,7 +263,7 @@ R p = getPreviousCompletedBuild(); if (p != null && isBuilding()) { Result pr = p.getResult(); - if (pr!= null && pr.isWorseThan(Result.UNSTABLE)) { + if (pr!= null && pr.isWorseOrEqualTo(Result.UNSTABLE)) { // we are still building, so this is just the current latest information, // but we seems to be failing so far, so inherit culprits from the previous build. // isBuilding() check is to avoid recursion when loading data from old Hudson, which doesn't record
          Hide
          mindless Alan Harder added a comment -

          looks ok to me.

          Show
          mindless Alan Harder added a comment - looks ok to me.
          Hide
          mcrooney mcrooney added a comment -

          Alas, it doesn't seem to be working but I'm not positive. This area was really confusing, especially how the block I changed is only executed if the previous build is building it seems. If anyone with some more insight wants to give that function (getCulprits) a peek to see, it would be appreciated.

          Show
          mcrooney mcrooney added a comment - Alas, it doesn't seem to be working but I'm not positive. This area was really confusing, especially how the block I changed is only executed if the previous build is building it seems. If anyone with some more insight wants to give that function (getCulprits) a peek to see, it would be appreciated.
          Hide
          mindless Alan Harder added a comment -

          yes, that code is odd.. looks like it relies on the fact that getCulprits is called further down in AbstractBuild.java from post().. isBuilding() will be true in this call, right?

          Show
          mindless Alan Harder added a comment - yes, that code is odd.. looks like it relies on the fact that getCulprits is called further down in AbstractBuild.java from post().. isBuilding() will be true in this call, right?
          Hide
          m211 m211 added a comment -

          In the getCulprits()-method the line
          Map <AbstractProject,AbstractBuild.DependencyChange> depmap = getDependencyChanges(getPreviousNotFailedBuild());
          may be wrong.
          I thing the
          getPreviousNotFailedBuild()
          must be replaced with
          getPreviousSuccessfulBuild()
          The getPreviousNotFailedBuild() provides the last unstable build. This can be the previous build. And then getDependencyChanges(..) provides only the dependency to the previous build and not to the last successful build. And the culprits from all previous unstable builds until to the last successful build will not be found.

          Show
          m211 m211 added a comment - In the getCulprits()-method the line Map <AbstractProject,AbstractBuild.DependencyChange> depmap = getDependencyChanges(getPreviousNotFailedBuild()); may be wrong. I thing the getPreviousNotFailedBuild() must be replaced with getPreviousSuccessfulBuild() The getPreviousNotFailedBuild() provides the last unstable build. This can be the previous build. And then getDependencyChanges(..) provides only the dependency to the previous build and not to the last successful build. And the culprits from all previous unstable builds until to the last successful build will not be found.
          Hide
          m211 m211 added a comment -

          for me it works.
          here is the patch.

          diff --git a/core/src/main/java/hudson/model/AbstractBuild.java b/core/src/main/java/hudson/model/AbstractBuild.java
          index 1b776f1..b1f4144 100644
          --- a/core/src/main/java/hudson/model/AbstractBuild.java
          +++ b/core/src/main/java/hudson/model/AbstractBuild.java
          @@ -278,7 +278,7 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
                       if (upstreamCulprits) {
                           // If we have dependencies since the last successful build, add their authors to our list
                           if (getPreviousNotFailedBuild() != null) {
          -                    Map <AbstractProject,AbstractBuild.DependencyChange> depmap = getDependencyChanges(getPreviousNotFailedBuild());
          +                    Map <AbstractProject,AbstractBuild.DependencyChange> depmap = getDependencyChanges(getPreviousSuccessfulBuild());
                               for (AbstractBuild.DependencyChange dep : depmap.values()) {
                                   for (AbstractBuild<?,?> b : dep.getBuilds()) {
                                       for (Entry entry : b.getChangeSet()) {
          
          Show
          m211 m211 added a comment - for me it works. here is the patch. diff --git a/core/src/main/java/hudson/model/AbstractBuild.java b/core/src/main/java/hudson/model/AbstractBuild.java index 1b776f1..b1f4144 100644 --- a/core/src/main/java/hudson/model/AbstractBuild.java +++ b/core/src/main/java/hudson/model/AbstractBuild.java @@ -278,7 +278,7 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs if (upstreamCulprits) { // If we have dependencies since the last successful build, add their authors to our list if (getPreviousNotFailedBuild() != null) { - Map <AbstractProject,AbstractBuild.DependencyChange> depmap = getDependencyChanges(getPreviousNotFailedBuild()); + Map <AbstractProject,AbstractBuild.DependencyChange> depmap = getDependencyChanges(getPreviousSuccessfulBuild()); for (AbstractBuild.DependencyChange dep : depmap.values()) { for (AbstractBuild<?,?> b : dep.getBuilds()) { for (Entry entry : b.getChangeSet()) {
          Hide
          m211 m211 added a comment -

          has anyone tried my patch?
          can the patch commited to hudson-core?

          Show
          m211 m211 added a comment - has anyone tried my patch? can the patch commited to hudson-core?
          Hide
          mwebber mwebber added a comment -

          I notice you didn't get any response to the question "has anyone tried my patch?", so I guess you should release it and folks like me can give some feedback. I'm not in a position to test patches.

          Show
          mwebber mwebber added a comment - I notice you didn't get any response to the question "has anyone tried my patch?", so I guess you should release it and folks like me can give some feedback. I'm not in a position to test patches.
          Hide
          mindless Alan Harder added a comment -

          m211, that change is only in the "if (upstreamCulprits)" block.. have you tested the simple case where there is no upstream job?

          Show
          mindless Alan Harder added a comment - m211, that change is only in the "if (upstreamCulprits)" block.. have you tested the simple case where there is no upstream job?
          Hide
          m211 m211 added a comment -

          yes it works also for the simple case without any upstream job.

          Show
          m211 m211 added a comment - yes it works also for the simple case without any upstream job.
          Hide
          scm_issue_link SCM/JIRA link daemon added a comment -

          Code changed in jenkins
          User: marco
          Path:
          core/src/main/java/hudson/model/AbstractBuild.java
          http://jenkins-ci.org/commit/jenkins/502219cf1b8be65f91d4ebd81e590558eb8c5b26
          Log:
          JENKINS-4617
          we are looking for dependencies since the last successful build
          and not since the last previous not failed build

          Show
          scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: marco Path: core/src/main/java/hudson/model/AbstractBuild.java http://jenkins-ci.org/commit/jenkins/502219cf1b8be65f91d4ebd81e590558eb8c5b26 Log: JENKINS-4617 we are looking for dependencies since the last successful build and not since the last previous not failed build
          Hide
          scm_issue_link SCM/JIRA link daemon added a comment -

          Code changed in jenkins
          User: Kohsuke Kawaguchi
          Path:
          changelog.html
          core/src/main/java/hudson/model/AbstractBuild.java
          test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java
          test/src/test/groovy/hudson/model/AbstractBuildTest.groovy
          http://jenkins-ci.org/commit/jenkins/e761d80f4d53a0f368fa8e9fbb06f01db247f8cc
          Log:
          [FIXED JENKINS-4617]
          this and the previous commit constitutes the fix.
          Also added a test case.

          Compare: https://github.com/jenkinsci/jenkins/compare/ad646cd...e761d80

          Show
          scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Kohsuke Kawaguchi Path: changelog.html core/src/main/java/hudson/model/AbstractBuild.java test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java test/src/test/groovy/hudson/model/AbstractBuildTest.groovy http://jenkins-ci.org/commit/jenkins/e761d80f4d53a0f368fa8e9fbb06f01db247f8cc Log: [FIXED JENKINS-4617] this and the previous commit constitutes the fix. Also added a test case. Compare: https://github.com/jenkinsci/jenkins/compare/ad646cd...e761d80
          Hide
          dogfood dogfood added a comment -

          Integrated in jenkins_main_trunk #683
          JENKINS-4617
          [FIXED JENKINS-4617]

          Kohsuke Kawaguchi : 502219cf1b8be65f91d4ebd81e590558eb8c5b26
          Files :

          • core/src/main/java/hudson/model/AbstractBuild.java

          Kohsuke Kawaguchi : e761d80f4d53a0f368fa8e9fbb06f01db247f8cc
          Files :

          • test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java
          • test/src/test/groovy/hudson/model/AbstractBuildTest.groovy
          • core/src/main/java/hudson/model/AbstractBuild.java
          • changelog.html
          Show
          dogfood dogfood added a comment - Integrated in jenkins_main_trunk #683 JENKINS-4617 [FIXED JENKINS-4617] Kohsuke Kawaguchi : 502219cf1b8be65f91d4ebd81e590558eb8c5b26 Files : core/src/main/java/hudson/model/AbstractBuild.java Kohsuke Kawaguchi : e761d80f4d53a0f368fa8e9fbb06f01db247f8cc Files : test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java test/src/test/groovy/hudson/model/AbstractBuildTest.groovy core/src/main/java/hudson/model/AbstractBuild.java changelog.html
          Hide
          scm_issue_link SCM/JIRA link daemon added a comment -

          Code changed in jenkins
          User: marco
          Path:
          core/src/main/java/hudson/model/AbstractBuild.java
          http://jenkins-ci.org/commit/jenkins/502219cf1b8be65f91d4ebd81e590558eb8c5b26
          Log:
          JENKINS-4617
          we are looking for dependencies since the last successful build
          and not since the last previous not failed build

          Show
          scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: marco Path: core/src/main/java/hudson/model/AbstractBuild.java http://jenkins-ci.org/commit/jenkins/502219cf1b8be65f91d4ebd81e590558eb8c5b26 Log: JENKINS-4617 we are looking for dependencies since the last successful build and not since the last previous not failed build
          Hide
          scm_issue_link SCM/JIRA link daemon added a comment -

          Code changed in jenkins
          User: Kohsuke Kawaguchi
          Path:
          changelog.html
          core/src/main/java/hudson/model/AbstractBuild.java
          test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java
          test/src/test/groovy/hudson/model/AbstractBuildTest.groovy
          http://jenkins-ci.org/commit/jenkins/e761d80f4d53a0f368fa8e9fbb06f01db247f8cc
          Log:
          [FIXED JENKINS-4617]
          this and the previous commit constitutes the fix.
          Also added a test case.

          Show
          scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Kohsuke Kawaguchi Path: changelog.html core/src/main/java/hudson/model/AbstractBuild.java test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java test/src/test/groovy/hudson/model/AbstractBuildTest.groovy http://jenkins-ci.org/commit/jenkins/e761d80f4d53a0f368fa8e9fbb06f01db247f8cc Log: [FIXED JENKINS-4617] this and the previous commit constitutes the fix. Also added a test case.
          Hide
          scm_issue_link SCM/JIRA link daemon added a comment -

          Code changed in jenkins
          User: marco
          Path:
          core/src/main/java/hudson/model/AbstractBuild.java
          http://jenkins-ci.org/commit/jenkins/502219cf1b8be65f91d4ebd81e590558eb8c5b26
          Log:
          JENKINS-4617
          we are looking for dependencies since the last successful build
          and not since the last previous not failed build

          Show
          scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: marco Path: core/src/main/java/hudson/model/AbstractBuild.java http://jenkins-ci.org/commit/jenkins/502219cf1b8be65f91d4ebd81e590558eb8c5b26 Log: JENKINS-4617 we are looking for dependencies since the last successful build and not since the last previous not failed build
          Hide
          scm_issue_link SCM/JIRA link daemon added a comment -

          Code changed in jenkins
          User: Kohsuke Kawaguchi
          Path:
          changelog.html
          core/src/main/java/hudson/model/AbstractBuild.java
          test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java
          test/src/test/groovy/hudson/model/AbstractBuildTest.groovy
          http://jenkins-ci.org/commit/jenkins/e761d80f4d53a0f368fa8e9fbb06f01db247f8cc
          Log:
          [FIXED JENKINS-4617]
          this and the previous commit constitutes the fix.
          Also added a test case.

          Show
          scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Kohsuke Kawaguchi Path: changelog.html core/src/main/java/hudson/model/AbstractBuild.java test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java test/src/test/groovy/hudson/model/AbstractBuildTest.groovy http://jenkins-ci.org/commit/jenkins/e761d80f4d53a0f368fa8e9fbb06f01db247f8cc Log: [FIXED JENKINS-4617] this and the previous commit constitutes the fix. Also added a test case.

            People

            • Assignee:
              markltbaker markltbaker
              Reporter:
              mcrooney mcrooney
            • Votes:
              6 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: