Skip to content

Add JS (lib/) subsystem labels #43

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 13, 2016
Merged

Add JS (lib/) subsystem labels #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
60faaf4
Labels: add JS Subsystem labels
Fishrock123 May 12, 2016
35657c0
Labels: use "lib / src" when 5+ JS Subsystems
Fishrock123 May 12, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 51 additions & 8 deletions lib/node-labels.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

This comment was marked as off-topic.

Sign in to view

[/^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'],

This comment was marked as off-topic.

Sign in to view

[/^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) {
Expand All @@ -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
}, {})

Expand Down
133 changes: 133 additions & 0 deletions test/node-js-subsystem-labels.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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()
})
22 changes: 17 additions & 5 deletions test/node-labels.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
])

Expand Down Expand Up @@ -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()
})
Expand Down