From cd7f2dd462ed75eb70c0ed652f80188e2b815978 Mon Sep 17 00:00:00 2001 From: Phillip Johnsen Date: Sat, 6 May 2017 13:39:22 +0200 Subject: [PATCH 1/2] labels: fix lib/src bucket for src/ changes This makes the `lib / src` bucket label also include files changes in `src/`. Previously it only was exclusive for `lib/` files. --- lib/node-labels.js | 30 +++++++++++++++--------------- test/unit/node-labels.test.js | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 15 deletions(-) diff --git a/lib/node-labels.js b/lib/node-labels.js index 87d4e924..29686fcb 100644 --- a/lib/node-labels.js +++ b/lib/node-labels.js @@ -130,19 +130,19 @@ const exclusiveLabelsMap = new Map([ [/^benchmark\//, 'benchmark'] ]) -function resolveLabels (filepathsChanged, baseBranch, limitLib = true) { +function resolveLabels (filepathsChanged, baseBranch, limitLabels = true) { const exclusiveLabels = matchExclusiveSubSystem(filepathsChanged) if (typeof baseBranch !== 'string') { if (typeof baseBranch === 'boolean') { - limitLib = baseBranch + limitLabels = baseBranch } baseBranch = '' } const labels = (exclusiveLabels.length > 0) ? exclusiveLabels - : matchAllSubSystem(filepathsChanged, limitLib) + : matchAllSubSystem(filepathsChanged, limitLabels) // Add version labels if PR is made against a version branch const m = /^(v\d+\.(?:\d+|x))(?:-staging|$)/.exec(baseBranch) @@ -195,13 +195,13 @@ function matchExclusiveSubSystem (filepathsChanged) { return isExclusive ? labels : [] } -function matchAllSubSystem (filepathsChanged, limitLib) { +function matchAllSubSystem (filepathsChanged, limitLabels) { return matchSubSystemsByRegex( - subSystemLabelsMap, filepathsChanged, limitLib) + subSystemLabelsMap, filepathsChanged, limitLabels) } -function matchSubSystemsByRegex (rxLabelsMap, filepathsChanged, limitLib) { - const jsLabelCount = [] +function matchSubSystemsByRegex (rxLabelsMap, filepathsChanged, limitLabels) { + const labelCount = [] // by putting matched labels into a map, we avoid duplicate labels const labelsMap = filepathsChanged.reduce((map, filepath) => { const mappedSubSystems = mappedSubSystemsForFile(rxLabelsMap, filepath) @@ -213,16 +213,17 @@ function matchSubSystemsByRegex (rxLabelsMap, filepathsChanged, limitLib) { for (var i = 0; i < mappedSubSystems.length; ++i) { const mappedSubSystem = mappedSubSystems[i] - if (limitLib && hasJsSubsystemChanges(filepathsChanged, mappedSubSystem)) { - if (jsLabelCount.length >= 4) { - for (const jsLabel of jsLabelCount) { - delete map[jsLabel] + if (limitLabels && hasLibOrSrcChanges(filepathsChanged)) { + if (labelCount.length >= 4) { + for (const label of labelCount) { + // don't delete the c++ label as we always want that if it has matched + if (label !== 'c++') delete map[label] } map['lib / src'] = true // short-circuit return map } else { - jsLabelCount.push(mappedSubSystem) + labelCount.push(mappedSubSystem) } } @@ -235,9 +236,8 @@ function matchSubSystemsByRegex (rxLabelsMap, filepathsChanged, limitLib) { return Object.keys(labelsMap) } -function hasJsSubsystemChanges (filepathsChanged, mappedSubSystem) { - const hasLibChanges = filepathsChanged.some((filepath) => filepath.startsWith('lib/')) - return hasLibChanges && jsSubsystemList.includes(mappedSubSystem) +function hasLibOrSrcChanges (filepathsChanged) { + return filepathsChanged.some((filepath) => filepath.startsWith('lib/') || filepath.startsWith('src/')) } function mappedSubSystemsForFile (labelsMap, filepath) { diff --git a/test/unit/node-labels.test.js b/test/unit/node-labels.test.js index 892c8df3..6c498c92 100644 --- a/test/unit/node-labels.test.js +++ b/test/unit/node-labels.test.js @@ -235,6 +235,39 @@ tap.test('label: "lib / src" when 5 or more JS sub-systems have been changed', ( t.end() }) +// https://github.com/nodejs/node/pull/12366 should have been labelled "lib / src" +// https://github.com/nodejs/github-bot/issues/137 +tap.test('label: "lib / src" when 5 or more native files have been changed', (t) => { + const labels = nodeLabels.resolveLabels([ + 'node.gyp', + 'src/cares_wrap.cc', + 'src/fs_event_wrap.cc', + 'src/node.cc', + 'src/node_api.cc', + 'src/node_buffer.cc', + 'src/node_config.cc', + 'src/node_constants.cc', + 'src/node_contextify.cc', + 'src/node_file.cc', + 'src/node_file.h', + 'src/node_http_parser.cc', + 'src/node_http_parser.h', + 'src/node_i18n.cc', + 'src/node_revert.cc', + 'src/node_serdes.cc', + 'src/node_zlib.cc', + 'src/process_wrap.cc', + 'src/signal_wrap.cc', + 'src/string_bytes.cc', + 'src/timer_wrap.cc', + 'src/uv.cc' + ]) + + t.same(labels, ['c++', 'lib / src']) + + t.end() +}) + // https://github.com/nodejs/node/pull/7488 wrongfully labelled with "lib / src" tap.test('label: not "lib / src" when only deps have been changed', (t) => { const labels = nodeLabels.resolveLabels([ From fa7d1fc56564641a28bd786d98aba91811c0ef64 Mon Sep 17 00:00:00 2001 From: Phillip Johnsen Date: Sat, 6 May 2017 16:48:48 +0200 Subject: [PATCH 2/2] labels: fix misleading test descriptions related to lib/src bucket --- test/unit/node-labels.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/unit/node-labels.test.js b/test/unit/node-labels.test.js index 6c498c92..88356e74 100644 --- a/test/unit/node-labels.test.js +++ b/test/unit/node-labels.test.js @@ -221,7 +221,7 @@ tap.test('label: "repl" when ./lib/repl.js has been changed', (t) => { t.end() }) -tap.test('label: "lib / src" when 5 or more JS sub-systems have been changed', (t) => { +tap.test('label: "lib / src" when 4 or more JS sub-systems have been changed', (t) => { const labels = nodeLabels.resolveLabels([ 'lib/assert.js', 'lib/dns.js', @@ -237,7 +237,7 @@ tap.test('label: "lib / src" when 5 or more JS sub-systems have been changed', ( // https://github.com/nodejs/node/pull/12366 should have been labelled "lib / src" // https://github.com/nodejs/github-bot/issues/137 -tap.test('label: "lib / src" when 5 or more native files have been changed', (t) => { +tap.test('label: "lib / src" when 4 or more native files have been changed', (t) => { const labels = nodeLabels.resolveLabels([ 'node.gyp', 'src/cares_wrap.cc', @@ -283,7 +283,7 @@ tap.test('label: not "lib / src" when only deps have been changed', (t) => { t.end() }) -tap.test('label: "JS sub-systems when less than 5 sub-systems have changed', (t) => { +tap.test('label: "JS sub-systems when less than 4 sub-systems have changed', (t) => { const labels = nodeLabels.resolveLabels([ 'lib/assert.js', 'lib/dns.js',