labels: fix lib/src bucket for src/ changes (#141)

This makes the `lib / src` bucket label also include files changes in `src/`. Previously it only was exclusive for `lib/` files.
nodejs · Jul 3, 2017 · bdfd21e · bdfd21e
1 parent 24a7945
commit bdfd21e
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 17 deletions.
diff --git a/lib/node-labels.js b/lib/node-labels.js
@@ -136,19 +136,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)
@@ -201,13 +201,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)
@@ -219,16 +219,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)
         }
       }
 
@@ -241,9 +242,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
@@ -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',
@@ -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 4 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([
@@ -250,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',