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
    • Similar Issues:
      Show 5 results

      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.

        Attachments

          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.
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Kohsuke Kawaguchi
            Path:
            test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java
            http://jenkins-ci.org/commit/jenkins-test-harness/10e915577cde1696703840c9c57014db63b7755f
            Log:
            [FIXED JENKINS-4617]
            this and the previous commit constitutes the fix.
            Also added a test case.

            Originally-Committed-As: e761d80f4d53a0f368fa8e9fbb06f01db247f8cc

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Kohsuke Kawaguchi Path: test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java http://jenkins-ci.org/commit/jenkins-test-harness/10e915577cde1696703840c9c57014db63b7755f Log: [FIXED JENKINS-4617] this and the previous commit constitutes the fix. Also added a test case. Originally-Committed-As: e761d80f4d53a0f368fa8e9fbb06f01db247f8cc

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: