Skip to content
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

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

Merged
merged 2 commits into from
Jul 3, 2017
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd7f2dd
labels: fix lib/src bucket for src/ changes
phillipj May 6, 2017
fa7d1fc
labels: fix misleading test descriptions related to lib/src bucket
phillipj May 6, 2017
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
30 changes: 15 additions & 15 deletions lib/node-labels.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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)
}
}

Expand All @@ -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) {
Expand Down
33 changes: 33 additions & 0 deletions test/unit/node-labels.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

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'])

This comment was marked as off-topic.

Sign in to view


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([
Expand Down