diff --git a/lib/node-labels.js b/lib/node-labels.js index fb1e7698..9874754a 100644 --- a/lib/node-labels.js +++ b/lib/node-labels.js @@ -9,24 +9,49 @@ const subSystemLabelsMap = new Map([ [/^([A-Z]+$|CODE_OF_CONDUCT|ROADMAP|WORKING_GROUPS|GOVERNANCE|CHANGELOG|\.mail|\.git.+)/, 'meta'], // things that edit top-level .md files are always a doc change [/^\w+\.md$/, 'doc'], + + /* Dependencies */ // libuv needs an explicit mapping, as the ordinary /deps/ mapping below would // end up as libuv changes labeled with "uv" (which is a non-existing label) [/^deps\/uv\//, 'libuv'], - [/^deps\/([^/]+)/, '$1'] + [/^deps\/([^/]+)/, '$1'], + + /* JS subsystems */ + // Oddities first + [/^lib\/(punycode|\w+\/freelist|sys\.js)/, ''], // TODO: ignore better? + [/^lib\/string_decoder/, 'buffer'], + [/^lib\/constants\.js$/, 'lib / src'], + [/^lib\/_(debug_agent|debugger)\.js$/, 'debugger'], + [/^lib(\/\w+)?\/(_)?link(ed)?list/, 'timers'], + [/^lib\/\w+\/bootstrap_node/, 'lib / src'], + [/^lib\/\w+\/v8_prof_/, 'tools'], + [/^lib\/\w+\/socket_list/, 'net'], + [/^lib\/\w+\/streams$/, 'stream'], + // All other lib/ files map directly + [/^lib\/_(\w+)_\w+\.js?$/, '$1'], // e.g. _(stream)_wrap + [/^lib(\/internal)?\/(\w+)\.js?$/, '$2'], // other .js files + [/^lib\/internal\/(\w+)$/, '$1'] // internal subfolders ]) +const jsSubsystemList = [ + 'debugger', 'assert', 'buffer', 'child_process', 'cluster', 'console', + 'crypto', 'dgram', 'dns', 'domain', 'events', 'fs', 'http', 'https', 'module', + 'net', 'os', 'path', 'process', 'querystring', 'readline', 'repl', 'stream', + 'timers', 'tls', 'tty', 'url', 'util', 'v8', 'vm', 'zlib' +] + const exclusiveLabelsMap = new Map([ [/^test\//, 'test'], [/^doc\//, 'doc'], [/^benchmark\//, 'benchmark'] ]) -function resolveLabels (filepathsChanged) { +function resolveLabels (filepathsChanged, limitLib = true) { const exclusiveLabels = matchExclusiveSubSystem(filepathsChanged) return (exclusiveLabels.length > 0) ? exclusiveLabels - : matchAllSubSystem(filepathsChanged) + : matchAllSubSystem(filepathsChanged, limitLib) } function matchExclusiveSubSystem (filepathsChanged) { @@ -35,19 +60,37 @@ function matchExclusiveSubSystem (filepathsChanged) { return (isExclusive && labels.length === 1) ? labels : [] } -function matchAllSubSystem (filepathsChanged) { - return matchSubSystemsByRegex(subSystemLabelsMap, filepathsChanged) +function matchAllSubSystem (filepathsChanged, limitLib) { + return matchSubSystemsByRegex( + subSystemLabelsMap, filepathsChanged, limitLib) } -function matchSubSystemsByRegex (rxLabelsMap, filepathsChanged) { +function matchSubSystemsByRegex (rxLabelsMap, filepathsChanged, limitLib) { + const jsLabelCount = [] // by putting matched labels into a map, we avoid duplicate labels const labelsMap = filepathsChanged.reduce((map, filepath) => { const mappedSubSystem = mappedSubSystemForFile(rxLabelsMap, filepath) - if (mappedSubSystem) { - map[mappedSubSystem] = true + if (!mappedSubSystem) { + // short-circuit + return map } + if (limitLib && jsSubsystemList.includes(mappedSubSystem)) { + if (jsLabelCount.length >= 4) { + for (const jsLabel of jsLabelCount) { + delete map[jsLabel] + } + map['lib / src'] = true + // short-circuit + return map + } else { + jsLabelCount.push(mappedSubSystem) + } + } + + map[mappedSubSystem] = true + return map }, {}) diff --git a/test/node-js-subsystem-labels.test.js b/test/node-js-subsystem-labels.test.js new file mode 100644 index 00000000..1640c1e1 --- /dev/null +++ b/test/node-js-subsystem-labels.test.js @@ -0,0 +1,133 @@ +'use strict' + +const tap = require('tap') + +const nodeLabels = require('../lib/node-labels') + +tap.test('label: lib oddities', (t) => { + const libFiles = [ + 'lib/_debug_agent.js', + 'lib/_http_agent.js', + 'lib/_http_client.js', + 'lib/_http_common.js', + 'lib/_http_incoming.js', + 'lib/_http_outgoing.js', + 'lib/_http_server.js', + 'lib/_linklist.js', + 'lib/_stream_duplex.js', + 'lib/_stream_passthrough.js', + 'lib/_stream_readable.js', + 'lib/_stream_transform.js', + 'lib/_stream_wrap.js', + 'lib/_stream_writable.js', + 'lib/_tls_common.js', + 'lib/_tls_legacy.js', + 'lib/_tls_wrap.js', + 'lib/constants.js', + 'lib/punycode.js', // ignored + 'lib/string_decoder.js', + 'lib/sys.js', // ignored + 'lib/internal/freelist.js', // ignored + 'lib/internal/process', + 'lib/internal/readme.md', // ignored + 'lib/internal/socket_list.js', + 'lib/internal/v8_prof_polyfill.js', + 'lib/internal/v8_prof_processor.js' + ] + + const labels = nodeLabels.resolveLabels(libFiles, false) + + t.same(labels, [ + 'debugger', // _debug_agent + 'http', // _http_* + 'timers', // linklist + 'stream', // _stream_* + 'tls', // _tls_* + 'lib / src', // constants + 'buffer', // string_decoder + 'process', // internal/process/ + 'net', // socket_list + 'tools' // v8_prof_* + ]) + + t.end() +}) + +tap.test('label: lib internals oddities duplicates', (t) => { + const libFiles = [ + 'lib/internal/bootstrap_node.js', + 'lib/internal/linkedlist.js', + 'lib/internal/streams' + ] + + const labels = nodeLabels.resolveLabels(libFiles) + + t.same(labels, [ + 'lib / src', // bootstrap_node + 'timers', // linkedlist + 'stream' // internal/streams/ + ]) + + t.end() +}) + +tap.test('label: lib normal without "lib / src" limiting', (t) => { + const libFiles = [ + 'lib/_debugger.js', + 'lib/assert.js', + 'lib/buffer.js', + 'lib/child_process.js', + 'lib/cluster.js', + 'lib/console.js', + 'lib/crypto.js', + 'lib/dgram.js', + 'lib/dns.js', + 'lib/domain.js', + 'lib/events.js', + 'lib/fs.js', + 'lib/http.js', + 'lib/https.js', + 'lib/module.js', + 'lib/net.js', + 'lib/os.js', + 'lib/path.js', + 'lib/process.js', + 'lib/querystring.js', + 'lib/readline.js', + 'lib/repl.js', + 'lib/stream.js', + 'lib/timers.js', + 'lib/tls.js', + 'lib/tty.js', + 'lib/url.js', + 'lib/util.js', + 'lib/v8.js', + 'lib/vm.js', + 'lib/zlib.js' + ] + + const labels = nodeLabels.resolveLabels(libFiles, false) + + t.same(labels, libFiles.map((path) => /lib\/(_)?(\w+)\.js/.exec(path)[2])) + + t.end() +}) + +tap.test('label: lib internals without "lib / src" limiting', (t) => { + const libFiles = [ + 'lib/internal/child_process.js', + 'lib/internal/cluster.js', + 'lib/internal/module.js', + 'lib/internal/net.js', + 'lib/internal/process.js', + 'lib/internal/readline.js', + 'lib/internal/repl.js', + 'lib/internal/util.js' + ] + + const labels = nodeLabels.resolveLabels(libFiles, false) + + t.same(labels, libFiles.map((path) => /lib\/internal\/(\w+)\.js/.exec(path)[1])) + + t.end() +}) diff --git a/test/node-labels.test.js b/test/node-labels.test.js index 792e011c..88259d19 100644 --- a/test/node-labels.test.js +++ b/test/node-labels.test.js @@ -32,7 +32,7 @@ tap.test('no labels: when ./test/ and ./doc/ files has been changed', (t) => { // https://github.com/nodejs/node/pull/6448 tap.test('no labels: when ./test/ and ./lib/ files has been changed', (t) => { const labels = nodeLabels.resolveLabels([ - 'lib/assert.js', + 'lib/punycode.js', 'test/parallel/test-assert.js' ]) @@ -128,22 +128,34 @@ tap.test('label: "repl" when ./lib/repl.js has been changed', (t) => { 'test/debugger/test-debugger-repl-term.js' ]) - t.same(labels, ['repl'], { todo: true }) + t.same(labels, ['repl']) t.end() }) -tap.test('label: "lib / src" when more than 5 sub-systems has been changed', (t) => { +tap.test('label: "lib / src" when 5 or more JS sub-systems have been changed', (t) => { const labels = nodeLabels.resolveLabels([ 'lib/assert.js', 'lib/dns.js', 'lib/repl.js', 'lib/process.js', - 'src/async-wrap.cc', 'lib/module.js' ]) - t.same(labels, ['lib / src'], { todo: true }) + t.same(labels, ['lib / src']) + + t.end() +}) + +tap.test('label: "JS sub-systems when less than 5 sub-systems have changed', (t) => { + const labels = nodeLabels.resolveLabels([ + 'lib/assert.js', + 'lib/dns.js', + 'lib/repl.js', + 'lib/process.js' + ]) + + t.same(labels, ['assert', 'dns', 'repl', 'process']) t.end() })