From f0b881469bdb63e2300ea68245219470c17d9c0c Mon Sep 17 00:00:00 2001 From: Mike Place Date: Fri, 5 Mar 2021 19:19:50 +0100 Subject: [PATCH 01/13] [WIP] Update to new schema for flaky tests --- src/co/elastic/NotificationManager.groovy | 98 ++++++++++++++++------- vars/notifyBuildResult.groovy | 4 - 2 files changed, 71 insertions(+), 31 deletions(-) diff --git a/src/co/elastic/NotificationManager.groovy b/src/co/elastic/NotificationManager.groovy index a6b0ce4c5..ad55fafe0 100644 --- a/src/co/elastic/NotificationManager.groovy +++ b/src/co/elastic/NotificationManager.groovy @@ -35,8 +35,6 @@ This method generates flakey test data from Jenkins test results * @param secret Vault path to secrets which hold authentication information for Elasticsearch * @param jobInfo JobInfo data collected from job-info.json * @param testsErrors list of test failed, see src/test/resources/tests-errors.json - * @param flakyReportIdx, what's the id. - * @param flakyThreshold, to tweak the score. * @param testsSummary object with the test results summary, see src/test/resources/tests-summary.json * @param querySize The maximum value of results to be reported. Default 500 * @param queryTimeout Specifies the period of time to wait for a response. Default 20s @@ -44,11 +42,11 @@ This method generates flakey test data from Jenkins test results * @param disableGHIssueCreation whether to disable the GH create issue if any flaky matches. */ def analyzeFlakey(Map args = [:]) { + print("Entering flake analyzero") + print(args) def es = args.containsKey('es') ? args.es : error('analyzeFlakey: es parameter is required') def secret = args.containsKey('es_secret') ? args.es_secret : null - def flakyReportIdx = args.containsKey('flakyReportIdx') ? args.flakyReportIdx : error('analyzeFlakey: flakyReportIdx parameter is required') def testsErrors = args.containsKey('testsErrors') ? args.testsErrors : [] - def flakyThreshold = args.containsKey('flakyThreshold') ? args.flakyThreshold : 0.0 def testsSummary = args.containsKey('testsSummary') ? args.testsSummary : null def querySize = args.get('querySize', 500) def queryTimeout = args.get('queryTimeout', '20s') @@ -60,32 +58,40 @@ def analyzeFlakey(Map args = [:]) { def flakyTestsWithIssues = [:] def genuineTestFailures = [] - if (!flakyReportIdx?.trim()) { - error 'analyzeFlakey: did not receive flakyReportIdx data' - } - // Only if there are test failures to analyse if(testsErrors.size() > 0) { // Query only the test_name field since it's the only used and don't want to overkill the // jenkins instance when using the toJSON step since it reads in memory the json response. // for 500 entries it's about 2500 lines versus 8000 lines if no filter_path - def query = "/${flakyReportIdx}/_search?size=${querySize}&filter_path=hits.hits._source.test_name,hits.hits._index" + //def query = "/flaky-tests/_search?size=${querySize}&filter_path=hits.hits._source.test_name,hits.hits._index" + def query = "/flaky-tests/_search?filter_path=aggregations.test_name.buckets" def flakeyTestsRaw = sendDataToElasticsearch(es: es, secret: secret, - data: queryFilter(queryTimeout, flakyThreshold), + data: queryFilter(queryTimeout), restCall: query) def flakeyTestsParsed = toJSON(flakeyTestsRaw) + print(flakeyTestsParsed) // Normalise both data structures with their names // Intesection what tests are failing and also scored as flaky. // Subset of genuine test failures, aka, those failures that were not scored as flaky previously. def testFailures = testsErrors.collect { it.name } - def testFlaky = flakeyTestsParsed?.hits?.hits?.collect { it['_source']['test_name'] } + def testFlaky = flakeyTestsParsed?.aggregations?.test_name?.buckets?.collect { it['key'] } + + // The following is code which is included here as a possible future enhancement if at some point + // we wish to include the number of flakes found in the time period. (Currently hard-coded to 90d) + // + //def testFlakyFreq = [:] + //flakeyTestsParsed?.aggregations?.test_name?.buckets?.each { it -> testFlakyFreq[it['key']] = it['doc_count'] } + def foundFlakyList = testFlaky?.size() > 0 ? testFailures.intersect(testFlaky) : [] genuineTestFailures = testFailures.minus(foundFlakyList) - log(level: 'DEBUG', text: "analyzeFlakey: Flaky tests raw: ${flakeyTestsRaw}") - log(level: 'DEBUG', text: "analyzeFlakey: Flaky matched tests: ${foundFlakyList.join('\n')}") + log(level: 'INFO', text: "analyzeFlakey: Flaky tests raw: ${flakeyTestsRaw}") + log(level: 'INFO', text: "analyzeFlakey: Flaky matched tests: ${foundFlakyList.join('\n')}") + + // FIXME this exits early to avoid creating actual issues during development + return def tests = lookForGitHubIssues(flakyList: foundFlakyList, labelsFilter: labels) // To avoid creating a few dozens of issues, let's say we won't create more than 3 issues per build @@ -367,19 +373,57 @@ def generateBuildReport(Map args = [:]) { return output } -def queryFilter(timeout, flakyThreshold) { - return """{ - "timeout": "${timeout}", - "sort" : [ - { "timestamp" : "desc" }, - { "test_score" : "desc" } - ], - "query" : { - "range" : { - "test_score" : { - "gt" : ${flakyThreshold} - } - } +def queryFilter(timeout) { + return """ +{ + "aggs": { + "test_name": { + "terms": { + "field": "test.name.keyword", + "order": { + "_count": "desc" + } + } + } + }, + "size": 0, + "script_fields": {}, + "stored_fields": [ + "*" + ], + "runtime_mappings": {}, + "_source": { + "excludes": [] + }, + "query": { + "bool": { + "must": [], + "filter": [ + { + "bool": { + "should": [ + { + "match_phrase": { + "job.fullName": "Beats/beats/master" } - }""" + } + ], + "minimum_should_match": 1 + } + }, + { + "range": { + "build.startTime": { + "gte": "now-90d", + "format": "strict_date_optional_time" + } + } + } + ], + "should": [], + "must_not": [] + } + } +} + """ } diff --git a/vars/notifyBuildResult.groovy b/vars/notifyBuildResult.groovy index 5a871f6f1..368e72177 100644 --- a/vars/notifyBuildResult.groovy +++ b/vars/notifyBuildResult.groovy @@ -52,8 +52,6 @@ def call(Map args = [:]) { def notifySlackComment = args.containsKey('slackComment') ? args.slackComment : false def analyzeFlakey = args.containsKey('analyzeFlakey') ? args.analyzeFlakey : false def newPRComment = args.containsKey('newPRComment') ? args.newPRComment : [:] - def flakyReportIdx = args.containsKey('flakyReportIdx') ? args.flakyReportIdx : "" - def flakyThreshold = args.containsKey('flakyThreshold') ? args.flakyThreshold : 0.0 node('master || metal || linux'){ stage('Reporting build status'){ @@ -75,8 +73,6 @@ def call(Map args = [:]) { data['statsUrl'] = statsURL data['es'] = es data['es_secret'] = secret - data['flakyReportIdx'] = flakyReportIdx - data['flakyThreshold'] = flakyThreshold data['header'] = slackHeader data['channel'] = slackChannel data['credentialId'] = slackCredentials From f1ddf69eb735b04a620344b1cf388ce42585e0c6 Mon Sep 17 00:00:00 2001 From: Mike Place Date: Tue, 9 Mar 2021 10:24:57 +0100 Subject: [PATCH 02/13] Remove unused timeout option --- src/co/elastic/NotificationManager.groovy | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/co/elastic/NotificationManager.groovy b/src/co/elastic/NotificationManager.groovy index ad55fafe0..52c4469d8 100644 --- a/src/co/elastic/NotificationManager.groovy +++ b/src/co/elastic/NotificationManager.groovy @@ -49,7 +49,6 @@ def analyzeFlakey(Map args = [:]) { def testsErrors = args.containsKey('testsErrors') ? args.testsErrors : [] def testsSummary = args.containsKey('testsSummary') ? args.testsSummary : null def querySize = args.get('querySize', 500) - def queryTimeout = args.get('queryTimeout', '20s') def disableGHComment = args.get('disableGHComment', false) def disableGHIssueCreation = args.get('disableGHIssueCreation', false) @@ -68,7 +67,7 @@ def analyzeFlakey(Map args = [:]) { def query = "/flaky-tests/_search?filter_path=aggregations.test_name.buckets" def flakeyTestsRaw = sendDataToElasticsearch(es: es, secret: secret, - data: queryFilter(queryTimeout), + data: queryFilter(), restCall: query) def flakeyTestsParsed = toJSON(flakeyTestsRaw) print(flakeyTestsParsed) @@ -373,7 +372,7 @@ def generateBuildReport(Map args = [:]) { return output } -def queryFilter(timeout) { +def queryFilter() { return """ { "aggs": { From 59d1a606acde10e2d5cca3fe029429ac1e5dc872 Mon Sep 17 00:00:00 2001 From: Mike Place Date: Tue, 9 Mar 2021 10:29:50 +0100 Subject: [PATCH 03/13] Remove early exit used for dev and use args instead --- src/co/elastic/NotificationManager.groovy | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/co/elastic/NotificationManager.groovy b/src/co/elastic/NotificationManager.groovy index 52c4469d8..2a6550ea5 100644 --- a/src/co/elastic/NotificationManager.groovy +++ b/src/co/elastic/NotificationManager.groovy @@ -89,9 +89,6 @@ def analyzeFlakey(Map args = [:]) { log(level: 'INFO', text: "analyzeFlakey: Flaky tests raw: ${flakeyTestsRaw}") log(level: 'INFO', text: "analyzeFlakey: Flaky matched tests: ${foundFlakyList.join('\n')}") - // FIXME this exits early to avoid creating actual issues during development - return - def tests = lookForGitHubIssues(flakyList: foundFlakyList, labelsFilter: labels) // To avoid creating a few dozens of issues, let's say we won't create more than 3 issues per build def numberOfSupportedIssues = 3 From a45584b8ea50d2633988dc3e9d537cf396c90e34 Mon Sep 17 00:00:00 2001 From: Mike Place Date: Tue, 9 Mar 2021 14:26:05 +0100 Subject: [PATCH 04/13] Adjust tests and args --- src/co/elastic/NotificationManager.groovy | 29 ++++++--- .../NotificationManagerStepTests.groovy | 64 +++++++++++-------- 2 files changed, 57 insertions(+), 36 deletions(-) diff --git a/src/co/elastic/NotificationManager.groovy b/src/co/elastic/NotificationManager.groovy index 2a6550ea5..8f615d0fa 100644 --- a/src/co/elastic/NotificationManager.groovy +++ b/src/co/elastic/NotificationManager.groovy @@ -45,6 +45,8 @@ def analyzeFlakey(Map args = [:]) { print("Entering flake analyzero") print(args) def es = args.containsKey('es') ? args.es : error('analyzeFlakey: es parameter is required') + def flakyReportIdx = args.containsKey('flakyReportIdx') ? args.flakyReportIdx : null + def jobName = args.containsKey('jobName') ? args.jobName : null def secret = args.containsKey('es_secret') ? args.es_secret : null def testsErrors = args.containsKey('testsErrors') ? args.testsErrors : [] def testsSummary = args.containsKey('testsSummary') ? args.testsSummary : null @@ -57,20 +59,27 @@ def analyzeFlakey(Map args = [:]) { def flakyTestsWithIssues = [:] def genuineTestFailures = [] - // Only if there are test failures to analyse - if(testsErrors.size() > 0) { + if (args.containsKey('flakyReportIdx') && args.containsKey('jobName')) { + error('Please pass either `jobName` or `flakyReportIdx` but not both') + } else if (flakyReportIdx == null && jobName == null) { + error('Must pass either flakyReportIdx or jobName') + } else if (flakyReportIdx != null) { + warnError message: "Please migrate from `flakyReportIdx` to `jobName`. Not collecting any results.", body: {} + return + } + + + // 1. Only if there are test failures to analyse + // 2. Only continue if we have a jobName passed in. This does not raise an error to preserve + // backward compatability. + if(testsErrors.size() > 0 && jobName != null) { - // Query only the test_name field since it's the only used and don't want to overkill the - // jenkins instance when using the toJSON step since it reads in memory the json response. - // for 500 entries it's about 2500 lines versus 8000 lines if no filter_path - //def query = "/flaky-tests/_search?size=${querySize}&filter_path=hits.hits._source.test_name,hits.hits._index" def query = "/flaky-tests/_search?filter_path=aggregations.test_name.buckets" def flakeyTestsRaw = sendDataToElasticsearch(es: es, secret: secret, - data: queryFilter(), + data: queryFilter(jobName), restCall: query) def flakeyTestsParsed = toJSON(flakeyTestsRaw) - print(flakeyTestsParsed) // Normalise both data structures with their names // Intesection what tests are failing and also scored as flaky. @@ -369,7 +378,7 @@ def generateBuildReport(Map args = [:]) { return output } -def queryFilter() { +def queryFilter(jobName) { return """ { "aggs": { @@ -400,7 +409,7 @@ def queryFilter() { "should": [ { "match_phrase": { - "job.fullName": "Beats/beats/master" + "job.fullName": "${jobName}" } } ], diff --git a/src/test/groovy/NotificationManagerStepTests.groovy b/src/test/groovy/NotificationManagerStepTests.groovy index aeaaca22b..c78b21989 100644 --- a/src/test/groovy/NotificationManagerStepTests.groovy +++ b/src/test/groovy/NotificationManagerStepTests.groovy @@ -701,7 +701,7 @@ class NotificationManagerStepTests extends ApmBasePipelineTest { helper.registerAllowedMethod('sendDataToElasticsearch', [Map.class], {readJSON(file: 'flake-empty-results.json')}) helper.registerAllowedMethod('isPR', { return true }) script.analyzeFlakey( - flakyReportIdx: 'reporter-apm-agent-python-apm-agent-python-master', + jobName: 'Beats/beats/master', es: "https://fake_url", testsErrors: readJSON(file: 'flake-tests-errors-without-match.json'), testsSummary: readJSON(file: 'flake-tests-summary.json') @@ -721,7 +721,7 @@ class NotificationManagerStepTests extends ApmBasePipelineTest { ) helper.registerAllowedMethod('isPR', { return true }) script.analyzeFlakey( - flakyReportIdx: 'reporter-apm-agent-python-apm-agent-python-master', + jobName: 'Beats/beats/master', es: "https://fake_url", testsErrors: readJSON(file: 'flake-tests-errors-without-match.json'), testsSummary: readJSON(file: 'flake-tests-summary.json') @@ -741,7 +741,7 @@ class NotificationManagerStepTests extends ApmBasePipelineTest { ) helper.registerAllowedMethod('isPR', { return true }) script.analyzeFlakey( - flakyReportIdx: 'reporter-apm-agent-python-apm-agent-python-master', + jobName: 'Beats/beats/master', es: "https://fake_url", testsErrors: readJSON(file: 'flake-tests-errors.json'), testsSummary: readJSON(file: 'flake-tests-summary.json') @@ -764,7 +764,7 @@ class NotificationManagerStepTests extends ApmBasePipelineTest { helper.registerAllowedMethod('githubCreateIssue', [Map.class], { return '100' } ) helper.registerAllowedMethod('isPR', { return true }) script.analyzeFlakey( - flakyReportIdx: 'reporter-apm-agent-python-apm-agent-python-master', + jobName: 'Beats/beats/master', es: "https://fake_url", testsErrors: readJSON(file: 'flake-tests-errors.json'), testsSummary: readJSON(file: 'flake-tests-summary.json') @@ -788,7 +788,7 @@ class NotificationManagerStepTests extends ApmBasePipelineTest { helper.registerAllowedMethod('githubCreateIssue', [Map.class], { return '100' } ) helper.registerAllowedMethod('isPR', { return true }) script.analyzeFlakey( - flakyReportIdx: 'reporter-apm-agent-python-apm-agent-python-master', + jobName: 'Beats/beats/master', es: "https://fake_url", testsErrors: readJSON(file: 'flake-tests-errors.json'), testsSummary: readJSON(file: 'flake-tests-summary.json') @@ -812,7 +812,7 @@ class NotificationManagerStepTests extends ApmBasePipelineTest { helper.registerAllowedMethod('githubCreateIssue', [Map.class], { return '100' } ) helper.registerAllowedMethod('isPR', { return true }) script.analyzeFlakey( - flakyReportIdx: 'reporter-apm-agent-python-apm-agent-python-master', + jobName: 'Beats/beats/master', es: "https://fake_url", testsErrors: readJSON(file: 'flake-tests-errors.json'), testsSummary: readJSON(file: 'flake-tests-summary.json') @@ -832,7 +832,7 @@ class NotificationManagerStepTests extends ApmBasePipelineTest { helper.registerAllowedMethod('lookForGitHubIssues', [Map.class], { return [ bar: '' ] }) helper.registerAllowedMethod('isPR', { return true }) script.analyzeFlakey( - flakyReportIdx: 'reporter-apm-agent-python-apm-agent-python-master', + jobName: 'Beats/beats/master', es: "https://fake_url", testsErrors: readJSON(file: 'flake-tests-errors.json'), testsSummary: readJSON(file: 'flake-tests-summary.json'), @@ -852,7 +852,7 @@ class NotificationManagerStepTests extends ApmBasePipelineTest { helper.registerAllowedMethod('lookForGitHubIssues', [Map.class], { return [:] } ) helper.registerAllowedMethod('isPR', { return true }) script.analyzeFlakey( - flakyReportIdx: 'reporter-apm-agent-python-apm-agent-python-master', + jobName: 'Beats/beats/master', es: "https://fake_url", testsErrors: [:], testsSummary: [ "failed": 0, "passed": 120, "skipped": 0, "total": 120 ] @@ -869,7 +869,7 @@ class NotificationManagerStepTests extends ApmBasePipelineTest { helper.registerAllowedMethod('lookForGitHubIssues', [Map.class], { return [:] } ) helper.registerAllowedMethod('isPR', { return true }) script.analyzeFlakey( - flakyReportIdx: 'reporter-apm-agent-python-apm-agent-python-master', + jobName: 'Beats/beats/master', es: "https://fake_url", testsErrors: [:], testsSummary: [:] @@ -880,28 +880,26 @@ class NotificationManagerStepTests extends ApmBasePipelineTest { } @Test - void test_analyzeFlakeyThreshold() throws Exception { - helper.registerAllowedMethod( - "sendDataToElasticsearch", - [Map.class], - {m -> readJSON(file: "flake-results.json")} - ) - script.analyzeFlakey( - flakyReportIdx: 'reporter-apm-agent-python-apm-agent-python-master', - es: "https://fake_url", - testsErrors: readJSON(file: 'flake-tests-errors.json'), - flakyThreshold: 0.5 - ) + void test_analyzeFlakeyNoJobInfo() throws Exception { + try { + script.analyzeFlakey( + flakyReportIdx: 'test', + es: "https://fake_url", + testsErrors: readJSON(file: 'flake-tests-errors.json') + ) + } catch(e) { + //NOOP + } printCallStack() - assertTrue(assertMethodCallContainsPattern('sendDataToElasticsearch', '"gt" : 0.5')) assertJobStatusSuccess() } @Test - void test_analyzeFlakeyNoJobInfo() throws Exception { + void test_analyzeFlakeyOldAndNewStyleArgs() throws Exception { try { script.analyzeFlakey( - flakyReportIdx: '', + flakyReportIdx: 'test', + jobName: 'testJobName', es: "https://fake_url", testsErrors: readJSON(file: 'flake-tests-errors.json') ) @@ -909,10 +907,24 @@ class NotificationManagerStepTests extends ApmBasePipelineTest { //NOOP } printCallStack() - assertTrue(assertMethodCallContainsPattern('error', 'analyzeFlakey: did not receive flakyReportIdx data')) + assertTrue(assertMethodCallContainsPattern('error', 'Please pass either `jobName` or `flakyReportIdx` but not both')) assertJobStatusFailure() } + @Test + void test_analyzeFlakeyNoIdxOrJobNameArgs() throws Exception { + try { + script.analyzeFlakey( + es: "https://fake_url", + testsErrors: readJSON(file: 'flake-tests-errors.json') + ) + } catch(e) { + //NOOP + } + printCallStack() + assertTrue(assertMethodCallContainsPatterns('error', 'Must pass either flakyReportIdx or jobName')) + } + @Test void test_analyzeFlakey_without_comment_notifications() throws Exception { helper.registerAllowedMethod('sendDataToElasticsearch', [Map.class], {readJSON(file: "flake-results.json")}) @@ -920,7 +932,7 @@ class NotificationManagerStepTests extends ApmBasePipelineTest { helper.registerAllowedMethod('isPR', { return true }) script.analyzeFlakey( disableGHComment: true, - flakyReportIdx: 'reporter-apm-agent-python-apm-agent-python-master', + jobName: 'Beats/beats/master', es: "https://fake_url", testsErrors: [:], testsSummary: [:] From 13fd37ffd9423eb24f2f4ea1c967f0481acb2f16 Mon Sep 17 00:00:00 2001 From: Mike Place Date: Tue, 9 Mar 2021 14:27:14 +0100 Subject: [PATCH 05/13] Remove debugging --- src/co/elastic/NotificationManager.groovy | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/co/elastic/NotificationManager.groovy b/src/co/elastic/NotificationManager.groovy index 8f615d0fa..4662b22b6 100644 --- a/src/co/elastic/NotificationManager.groovy +++ b/src/co/elastic/NotificationManager.groovy @@ -42,8 +42,6 @@ This method generates flakey test data from Jenkins test results * @param disableGHIssueCreation whether to disable the GH create issue if any flaky matches. */ def analyzeFlakey(Map args = [:]) { - print("Entering flake analyzero") - print(args) def es = args.containsKey('es') ? args.es : error('analyzeFlakey: es parameter is required') def flakyReportIdx = args.containsKey('flakyReportIdx') ? args.flakyReportIdx : null def jobName = args.containsKey('jobName') ? args.jobName : null From e6e74b79148b0dfa658c5874f99f63d47f5db97d Mon Sep 17 00:00:00 2001 From: Mike Place Date: Tue, 9 Mar 2021 15:37:48 +0100 Subject: [PATCH 06/13] Fix typo --- src/test/groovy/NotificationManagerStepTests.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/groovy/NotificationManagerStepTests.groovy b/src/test/groovy/NotificationManagerStepTests.groovy index c78b21989..cad1fecec 100644 --- a/src/test/groovy/NotificationManagerStepTests.groovy +++ b/src/test/groovy/NotificationManagerStepTests.groovy @@ -922,7 +922,7 @@ class NotificationManagerStepTests extends ApmBasePipelineTest { //NOOP } printCallStack() - assertTrue(assertMethodCallContainsPatterns('error', 'Must pass either flakyReportIdx or jobName')) + assertTrue(assertMethodCallContainsPattern('error', 'Must pass either flakyReportIdx or jobName')) } @Test From 0af3796a1c87e12f9ee5ec85d374f1143d4a8cc0 Mon Sep 17 00:00:00 2001 From: Mike Place Date: Tue, 9 Mar 2021 15:41:28 +0100 Subject: [PATCH 07/13] Update docs --- vars/notifyBuildResult.txt | 2 -- 1 file changed, 2 deletions(-) diff --git a/vars/notifyBuildResult.txt b/vars/notifyBuildResult.txt index 50750a61f..0d1022e02 100644 --- a/vars/notifyBuildResult.txt +++ b/vars/notifyBuildResult.txt @@ -35,8 +35,6 @@ emails on Failed builds that are not pull request. * slackCredentials: What slack credentials to be used. Default value uses `jenkins-slack-integration-token`. * slackNotify: Whether to send or not the slack notifications, by default it sends notifications on Failed builds that are not pull request. * analyzeFlakey: Whether or not to add a comment in the PR with tests which have been detected as flakey. Default: `false`. -* flakyReportIdx: The flaky index to compare this jobs results to. e.g. reporter-apm-agent-java-apm-agent-java-master -* flakyThreshold: The threshold below which flaky tests will be ignored. Default: 0.0 * flakyDisableGHIssueCreation: whether to disable the GH create issue if any flaky matches. Default false. * newPRComment: The map of the data to be populated as a comment. Default empty. * aggregateComments: Whether to create only one single GitHub PR Comment with all the details. Default true. From ce21387c1d2157a5528321c3719f45bc4e814485 Mon Sep 17 00:00:00 2001 From: Mike Place Date: Tue, 9 Mar 2021 17:03:05 +0100 Subject: [PATCH 08/13] Add jobName var --- src/co/elastic/NotificationManager.groovy | 2 +- vars/notifyBuildResult.groovy | 2 ++ vars/notifyBuildResult.txt | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/co/elastic/NotificationManager.groovy b/src/co/elastic/NotificationManager.groovy index 4662b22b6..c63ca731c 100644 --- a/src/co/elastic/NotificationManager.groovy +++ b/src/co/elastic/NotificationManager.groovy @@ -40,6 +40,7 @@ This method generates flakey test data from Jenkins test results * @param queryTimeout Specifies the period of time to wait for a response. Default 20s * @param disableGHComment whether to disable the GH comment notification. * @param disableGHIssueCreation whether to disable the GH create issue if any flaky matches. + * @param jobName */ def analyzeFlakey(Map args = [:]) { def es = args.containsKey('es') ? args.es : error('analyzeFlakey: es parameter is required') @@ -51,7 +52,6 @@ def analyzeFlakey(Map args = [:]) { def querySize = args.get('querySize', 500) def disableGHComment = args.get('disableGHComment', false) def disableGHIssueCreation = args.get('disableGHIssueCreation', false) - def labels = 'flaky-test,ci-reported' def boURL = getBlueoceanDisplayURL() def flakyTestsWithIssues = [:] diff --git a/vars/notifyBuildResult.groovy b/vars/notifyBuildResult.groovy index 368e72177..1dd8c2455 100644 --- a/vars/notifyBuildResult.groovy +++ b/vars/notifyBuildResult.groovy @@ -66,6 +66,7 @@ def call(Map args = [:]) { def slackCredentials = args.containsKey('slackCredentials') ? args.slackCredentials : 'jenkins-slack-integration-token' def aggregateComments = args.get('aggregateComments', true) def flakyDisableGHIssueCreation = args.get('flakyDisableGHIssueCreation', false) + def jobName = args.get('jobName') ? args.jobName : '' catchError(message: 'There were some failures with the notifications', buildResult: 'SUCCESS', stageResult: 'UNSTABLE') { def data = getBuildInfoJsonFiles(jobURL: env.JOB_URL, buildNumber: env.BUILD_NUMBER, returnData: true) data['docsUrl'] = "http://${env?.REPO_NAME}_${env?.CHANGE_ID}.docs-preview.app.elstc.co/diff" @@ -77,6 +78,7 @@ def call(Map args = [:]) { data['channel'] = slackChannel data['credentialId'] = slackCredentials data['enabled'] = slackNotify + data['jobName'] = jobName data['disableGHIssueCreation'] = flakyDisableGHIssueCreation // Allow to aggregate the comments, for such it disables the default notifications. diff --git a/vars/notifyBuildResult.txt b/vars/notifyBuildResult.txt index 0d1022e02..a11707f76 100644 --- a/vars/notifyBuildResult.txt +++ b/vars/notifyBuildResult.txt @@ -38,3 +38,4 @@ emails on Failed builds that are not pull request. * flakyDisableGHIssueCreation: whether to disable the GH create issue if any flaky matches. Default false. * newPRComment: The map of the data to be populated as a comment. Default empty. * aggregateComments: Whether to create only one single GitHub PR Comment with all the details. Default true. +* jobName: The name of the job, e.g. `Beats/beats/master`. \ No newline at end of file From 393e2b9cd8a174d5914715514d73ace28022a0d1 Mon Sep 17 00:00:00 2001 From: Mike Place Date: Mon, 15 Mar 2021 11:58:59 +0100 Subject: [PATCH 09/13] Fix lint error --- vars/notifyBuildResult.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vars/notifyBuildResult.txt b/vars/notifyBuildResult.txt index a11707f76..9e525b948 100644 --- a/vars/notifyBuildResult.txt +++ b/vars/notifyBuildResult.txt @@ -38,4 +38,4 @@ emails on Failed builds that are not pull request. * flakyDisableGHIssueCreation: whether to disable the GH create issue if any flaky matches. Default false. * newPRComment: The map of the data to be populated as a comment. Default empty. * aggregateComments: Whether to create only one single GitHub PR Comment with all the details. Default true. -* jobName: The name of the job, e.g. `Beats/beats/master`. \ No newline at end of file +* jobName: The name of the job, e.g. `Beats/beats/master`. From 710ae20a6016faa10c53add49ec6d057b3068fca Mon Sep 17 00:00:00 2001 From: Mike Place Date: Mon, 15 Mar 2021 12:17:59 +0100 Subject: [PATCH 10/13] Remove idx per review suggestion --- src/co/elastic/NotificationManager.groovy | 13 +----- .../NotificationManagerStepTests.groovy | 46 ------------------- 2 files changed, 1 insertion(+), 58 deletions(-) diff --git a/src/co/elastic/NotificationManager.groovy b/src/co/elastic/NotificationManager.groovy index c63ca731c..e4a597c5f 100644 --- a/src/co/elastic/NotificationManager.groovy +++ b/src/co/elastic/NotificationManager.groovy @@ -44,8 +44,7 @@ This method generates flakey test data from Jenkins test results */ def analyzeFlakey(Map args = [:]) { def es = args.containsKey('es') ? args.es : error('analyzeFlakey: es parameter is required') - def flakyReportIdx = args.containsKey('flakyReportIdx') ? args.flakyReportIdx : null - def jobName = args.containsKey('jobName') ? args.jobName : null + def jobName = args.containsKey('jobName') ? args.jobName : error('analyzeFlakey: jobName parameter is required') def secret = args.containsKey('es_secret') ? args.es_secret : null def testsErrors = args.containsKey('testsErrors') ? args.testsErrors : [] def testsSummary = args.containsKey('testsSummary') ? args.testsSummary : null @@ -57,16 +56,6 @@ def analyzeFlakey(Map args = [:]) { def flakyTestsWithIssues = [:] def genuineTestFailures = [] - if (args.containsKey('flakyReportIdx') && args.containsKey('jobName')) { - error('Please pass either `jobName` or `flakyReportIdx` but not both') - } else if (flakyReportIdx == null && jobName == null) { - error('Must pass either flakyReportIdx or jobName') - } else if (flakyReportIdx != null) { - warnError message: "Please migrate from `flakyReportIdx` to `jobName`. Not collecting any results.", body: {} - return - } - - // 1. Only if there are test failures to analyse // 2. Only continue if we have a jobName passed in. This does not raise an error to preserve // backward compatability. diff --git a/src/test/groovy/NotificationManagerStepTests.groovy b/src/test/groovy/NotificationManagerStepTests.groovy index cad1fecec..7b2630a18 100644 --- a/src/test/groovy/NotificationManagerStepTests.groovy +++ b/src/test/groovy/NotificationManagerStepTests.groovy @@ -879,52 +879,6 @@ class NotificationManagerStepTests extends ApmBasePipelineTest { assertJobStatusSuccess() } - @Test - void test_analyzeFlakeyNoJobInfo() throws Exception { - try { - script.analyzeFlakey( - flakyReportIdx: 'test', - es: "https://fake_url", - testsErrors: readJSON(file: 'flake-tests-errors.json') - ) - } catch(e) { - //NOOP - } - printCallStack() - assertJobStatusSuccess() - } - - @Test - void test_analyzeFlakeyOldAndNewStyleArgs() throws Exception { - try { - script.analyzeFlakey( - flakyReportIdx: 'test', - jobName: 'testJobName', - es: "https://fake_url", - testsErrors: readJSON(file: 'flake-tests-errors.json') - ) - } catch(e) { - //NOOP - } - printCallStack() - assertTrue(assertMethodCallContainsPattern('error', 'Please pass either `jobName` or `flakyReportIdx` but not both')) - assertJobStatusFailure() - } - - @Test - void test_analyzeFlakeyNoIdxOrJobNameArgs() throws Exception { - try { - script.analyzeFlakey( - es: "https://fake_url", - testsErrors: readJSON(file: 'flake-tests-errors.json') - ) - } catch(e) { - //NOOP - } - printCallStack() - assertTrue(assertMethodCallContainsPattern('error', 'Must pass either flakyReportIdx or jobName')) - } - @Test void test_analyzeFlakey_without_comment_notifications() throws Exception { helper.registerAllowedMethod('sendDataToElasticsearch', [Map.class], {readJSON(file: "flake-results.json")}) From 8e20e19cc6186c02fe09bccb98c17d7ac9bedb0f Mon Sep 17 00:00:00 2001 From: cachedout Date: Mon, 15 Mar 2021 14:01:38 +0000 Subject: [PATCH 11/13] Update src/co/elastic/NotificationManager.groovy Co-authored-by: Victor Martinez --- src/co/elastic/NotificationManager.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/co/elastic/NotificationManager.groovy b/src/co/elastic/NotificationManager.groovy index e4a597c5f..62528b20e 100644 --- a/src/co/elastic/NotificationManager.groovy +++ b/src/co/elastic/NotificationManager.groovy @@ -59,7 +59,7 @@ def analyzeFlakey(Map args = [:]) { // 1. Only if there are test failures to analyse // 2. Only continue if we have a jobName passed in. This does not raise an error to preserve // backward compatability. - if(testsErrors.size() > 0 && jobName != null) { + if(testsErrors.size() > 0 && jobName?.trim()) { def query = "/flaky-tests/_search?filter_path=aggregations.test_name.buckets" def flakeyTestsRaw = sendDataToElasticsearch(es: es, From d94587b67940acb471bb2809939d716f01c03636 Mon Sep 17 00:00:00 2001 From: cachedout Date: Mon, 15 Mar 2021 14:01:46 +0000 Subject: [PATCH 12/13] Update src/co/elastic/NotificationManager.groovy Co-authored-by: Victor Martinez --- src/co/elastic/NotificationManager.groovy | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/co/elastic/NotificationManager.groovy b/src/co/elastic/NotificationManager.groovy index 62528b20e..b8adc8701 100644 --- a/src/co/elastic/NotificationManager.groovy +++ b/src/co/elastic/NotificationManager.groovy @@ -82,8 +82,8 @@ def analyzeFlakey(Map args = [:]) { def foundFlakyList = testFlaky?.size() > 0 ? testFailures.intersect(testFlaky) : [] genuineTestFailures = testFailures.minus(foundFlakyList) - log(level: 'INFO', text: "analyzeFlakey: Flaky tests raw: ${flakeyTestsRaw}") - log(level: 'INFO', text: "analyzeFlakey: Flaky matched tests: ${foundFlakyList.join('\n')}") + log(level: 'DEBUG', text: "analyzeFlakey: Flaky tests raw: ${flakeyTestsRaw}") + log(level: 'DEBUG', text: "analyzeFlakey: Flaky matched tests: ${foundFlakyList.join('\n')}") def tests = lookForGitHubIssues(flakyList: foundFlakyList, labelsFilter: labels) // To avoid creating a few dozens of issues, let's say we won't create more than 3 issues per build From 52f46dce03da5537b734fe9b83abb34ec885911c Mon Sep 17 00:00:00 2001 From: Mike Place Date: Mon, 15 Mar 2021 15:49:05 +0100 Subject: [PATCH 13/13] Refine query a bit --- src/co/elastic/NotificationManager.groovy | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/co/elastic/NotificationManager.groovy b/src/co/elastic/NotificationManager.groovy index e4a597c5f..21be10544 100644 --- a/src/co/elastic/NotificationManager.groovy +++ b/src/co/elastic/NotificationManager.groovy @@ -379,14 +379,6 @@ def queryFilter(jobName) { } }, "size": 0, - "script_fields": {}, - "stored_fields": [ - "*" - ], - "runtime_mappings": {}, - "_source": { - "excludes": [] - }, "query": { "bool": { "must": [],