Skip to content

Commit d8f0a38

Browse files
Fishrock123phillipj
Fishrock123
authored and
phillipj
committed
Add JS (lib/) subsystem labels (#43)
* Labels: add JS Subsystem labels * Labels: use "lib / src" when 5+ JS Subsystems
1 parent ef93782 commit d8f0a38

File tree

3 files changed

+201
-13
lines changed

3 files changed

+201
-13
lines changed

‎lib/node-labels.js

Lines changed: 51 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,24 +9,49 @@ const subSystemLabelsMap = new Map([
99
[/^([A-Z]+$|CODE_OF_CONDUCT|ROADMAP|WORKING_GROUPS|GOVERNANCE|CHANGELOG|\.mail|\.git.+)/, 'meta'],
1010
// things that edit top-level .md files are always a doc change
1111
[/^\w+\.md$/, 'doc'],
12+
13+
/* Dependencies */
1214
// libuv needs an explicit mapping, as the ordinary /deps/ mapping below would
1315
// end up as libuv changes labeled with "uv" (which is a non-existing label)
1416
[/^deps\/uv\//, 'libuv'],
15-
[/^deps\/([^/]+)/, '$1']
17+
[/^deps\/([^/]+)/, '$1'],
18+
19+
/* JS subsystems */
20+
// Oddities first
21+
[/^lib\/(punycode|\w+\/freelist|sys\.js)/, ''], // TODO: ignore better?
22+
[/^lib\/string_decoder/, 'buffer'],
23+
[/^lib\/constants\.js$/, 'lib / src'],
24+
[/^lib\/_(debug_agent|debugger)\.js$/, 'debugger'],
25+
[/^lib(\/\w+)?\/(_)?link(ed)?list/, 'timers'],
26+
[/^lib\/\w+\/bootstrap_node/, 'lib / src'],
27+
[/^lib\/\w+\/v8_prof_/, 'tools'],
28+
[/^lib\/\w+\/socket_list/, 'net'],
29+
[/^lib\/\w+\/streams$/, 'stream'],
30+
// All other lib/ files map directly
31+
[/^lib\/_(\w+)_\w+\.js?$/, '$1'], // e.g. _(stream)_wrap
32+
[/^lib(\/internal)?\/(\w+)\.js?$/, '$2'], // other .js files
33+
[/^lib\/internal\/(\w+)$/, '$1'] // internal subfolders
1634
])
1735

36+
const jsSubsystemList = [
37+
'debugger', 'assert', 'buffer', 'child_process', 'cluster', 'console',
38+
'crypto', 'dgram', 'dns', 'domain', 'events', 'fs', 'http', 'https', 'module',
39+
'net', 'os', 'path', 'process', 'querystring', 'readline', 'repl', 'stream',
40+
'timers', 'tls', 'tty', 'url', 'util', 'v8', 'vm', 'zlib'
41+
]
42+
1843
const exclusiveLabelsMap = new Map([
1944
[/^test\//, 'test'],
2045
[/^doc\//, 'doc'],
2146
[/^benchmark\//, 'benchmark']
2247
])
2348

24-
function resolveLabels (filepathsChanged) {
49+
function resolveLabels (filepathsChanged, limitLib = true) {
2550
const exclusiveLabels = matchExclusiveSubSystem(filepathsChanged)
2651

2752
return (exclusiveLabels.length > 0)
2853
? exclusiveLabels
29-
: matchAllSubSystem(filepathsChanged)
54+
: matchAllSubSystem(filepathsChanged, limitLib)
3055
}
3156

3257
function matchExclusiveSubSystem (filepathsChanged) {
@@ -35,19 +60,37 @@ function matchExclusiveSubSystem (filepathsChanged) {
3560
return (isExclusive && labels.length === 1) ? labels : []
3661
}
3762

38-
function matchAllSubSystem (filepathsChanged) {
39-
return matchSubSystemsByRegex(subSystemLabelsMap, filepathsChanged)
63+
function matchAllSubSystem (filepathsChanged, limitLib) {
64+
return matchSubSystemsByRegex(
65+
subSystemLabelsMap, filepathsChanged, limitLib)
4066
}
4167

42-
function matchSubSystemsByRegex (rxLabelsMap, filepathsChanged) {
68+
function matchSubSystemsByRegex (rxLabelsMap, filepathsChanged, limitLib) {
69+
const jsLabelCount = []
4370
// by putting matched labels into a map, we avoid duplicate labels
4471
const labelsMap = filepathsChanged.reduce((map, filepath) => {
4572
const mappedSubSystem = mappedSubSystemForFile(rxLabelsMap, filepath)
4673

47-
if (mappedSubSystem) {
48-
map[mappedSubSystem] = true
74+
if (!mappedSubSystem) {
75+
// short-circuit
76+
return map
4977
}
5078

79+
if (limitLib && jsSubsystemList.includes(mappedSubSystem)) {
80+
if (jsLabelCount.length >= 4) {
81+
for (const jsLabel of jsLabelCount) {
82+
delete map[jsLabel]
83+
}
84+
map['lib / src'] = true
85+
// short-circuit
86+
return map
87+
} else {
88+
jsLabelCount.push(mappedSubSystem)
89+
}
90+
}
91+
92+
map[mappedSubSystem] = true
93+
5194
return map
5295
}, {})
5396

‎test/node-js-subsystem-labels.test.js

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
'use strict'
2+
3+
const tap = require('tap')
4+
5+
const nodeLabels = require('../lib/node-labels')
6+
7+
tap.test('label: lib oddities', (t) => {
8+
const libFiles = [
9+
'lib/_debug_agent.js',
10+
'lib/_http_agent.js',
11+
'lib/_http_client.js',
12+
'lib/_http_common.js',
13+
'lib/_http_incoming.js',
14+
'lib/_http_outgoing.js',
15+
'lib/_http_server.js',
16+
'lib/_linklist.js',
17+
'lib/_stream_duplex.js',
18+
'lib/_stream_passthrough.js',
19+
'lib/_stream_readable.js',
20+
'lib/_stream_transform.js',
21+
'lib/_stream_wrap.js',
22+
'lib/_stream_writable.js',
23+
'lib/_tls_common.js',
24+
'lib/_tls_legacy.js',
25+
'lib/_tls_wrap.js',
26+
'lib/constants.js',
27+
'lib/punycode.js', // ignored
28+
'lib/string_decoder.js',
29+
'lib/sys.js', // ignored
30+
'lib/internal/freelist.js', // ignored
31+
'lib/internal/process',
32+
'lib/internal/readme.md', // ignored
33+
'lib/internal/socket_list.js',
34+
'lib/internal/v8_prof_polyfill.js',
35+
'lib/internal/v8_prof_processor.js'
36+
]
37+
38+
const labels = nodeLabels.resolveLabels(libFiles, false)
39+
40+
t.same(labels, [
41+
'debugger', // _debug_agent
42+
'http', // _http_*
43+
'timers', // linklist
44+
'stream', // _stream_*
45+
'tls', // _tls_*
46+
'lib / src', // constants
47+
'buffer', // string_decoder
48+
'process', // internal/process/
49+
'net', // socket_list
50+
'tools' // v8_prof_*
51+
])
52+
53+
t.end()
54+
})
55+
56+
tap.test('label: lib internals oddities duplicates', (t) => {
57+
const libFiles = [
58+
'lib/internal/bootstrap_node.js',
59+
'lib/internal/linkedlist.js',
60+
'lib/internal/streams'
61+
]
62+
63+
const labels = nodeLabels.resolveLabels(libFiles)
64+
65+
t.same(labels, [
66+
'lib / src', // bootstrap_node
67+
'timers', // linkedlist
68+
'stream' // internal/streams/
69+
])
70+
71+
t.end()
72+
})
73+
74+
tap.test('label: lib normal without "lib / src" limiting', (t) => {
75+
const libFiles = [
76+
'lib/_debugger.js',
77+
'lib/assert.js',
78+
'lib/buffer.js',
79+
'lib/child_process.js',
80+
'lib/cluster.js',
81+
'lib/console.js',
82+
'lib/crypto.js',
83+
'lib/dgram.js',
84+
'lib/dns.js',
85+
'lib/domain.js',
86+
'lib/events.js',
87+
'lib/fs.js',
88+
'lib/http.js',
89+
'lib/https.js',
90+
'lib/module.js',
91+
'lib/net.js',
92+
'lib/os.js',
93+
'lib/path.js',
94+
'lib/process.js',
95+
'lib/querystring.js',
96+
'lib/readline.js',
97+
'lib/repl.js',
98+
'lib/stream.js',
99+
'lib/timers.js',
100+
'lib/tls.js',
101+
'lib/tty.js',
102+
'lib/url.js',
103+
'lib/util.js',
104+
'lib/v8.js',
105+
'lib/vm.js',
106+
'lib/zlib.js'
107+
]
108+
109+
const labels = nodeLabels.resolveLabels(libFiles, false)
110+
111+
t.same(labels, libFiles.map((path) => /lib\/(_)?(\w+)\.js/.exec(path)[2]))
112+
113+
t.end()
114+
})
115+
116+
tap.test('label: lib internals without "lib / src" limiting', (t) => {
117+
const libFiles = [
118+
'lib/internal/child_process.js',
119+
'lib/internal/cluster.js',
120+
'lib/internal/module.js',
121+
'lib/internal/net.js',
122+
'lib/internal/process.js',
123+
'lib/internal/readline.js',
124+
'lib/internal/repl.js',
125+
'lib/internal/util.js'
126+
]
127+
128+
const labels = nodeLabels.resolveLabels(libFiles, false)
129+
130+
t.same(labels, libFiles.map((path) => /lib\/internal\/(\w+)\.js/.exec(path)[1]))
131+
132+
t.end()
133+
})

‎test/node-labels.test.js

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ tap.test('no labels: when ./test/ and ./doc/ files has been changed', (t) => {
3232
// https://github.com/nodejs/node/pull/6448
3333
tap.test('no labels: when ./test/ and ./lib/ files has been changed', (t) => {
3434
const labels = nodeLabels.resolveLabels([
35-
'lib/assert.js',
35+
'lib/punycode.js',
3636
'test/parallel/test-assert.js'
3737
])
3838

@@ -128,22 +128,34 @@ tap.test('label: "repl" when ./lib/repl.js has been changed', (t) => {
128128
'test/debugger/test-debugger-repl-term.js'
129129
])
130130

131-
t.same(labels, ['repl'], { todo: true })
131+
t.same(labels, ['repl'])
132132

133133
t.end()
134134
})
135135

136-
tap.test('label: "lib / src" when more than 5 sub-systems has been changed', (t) => {
136+
tap.test('label: "lib / src" when 5 or more JS sub-systems have been changed', (t) => {
137137
const labels = nodeLabels.resolveLabels([
138138
'lib/assert.js',
139139
'lib/dns.js',
140140
'lib/repl.js',
141141
'lib/process.js',
142-
'src/async-wrap.cc',
143142
'lib/module.js'
144143
])
145144

146-
t.same(labels, ['lib / src'], { todo: true })
145+
t.same(labels, ['lib / src'])
146+
147+
t.end()
148+
})
149+
150+
tap.test('label: "JS sub-systems when less than 5 sub-systems have changed', (t) => {
151+
const labels = nodeLabels.resolveLabels([
152+
'lib/assert.js',
153+
'lib/dns.js',
154+
'lib/repl.js',
155+
'lib/process.js'
156+
])
157+
158+
t.same(labels, ['assert', 'dns', 'repl', 'process'])
147159

148160
t.end()
149161
})

0 commit comments

Comments
 (0)