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.

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

Achieve 100% test coverage #349

Merged
merged 22 commits into from
Mar 5, 2024
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
e39091e
test: Router module integration
jean-michelet Feb 20, 2024
0ab4156
fix: add proxyquire to dev dependencies
jean-michelet Feb 23, 2024
c9ccb25
test: cover uncovered branches in index.js
jean-michelet Feb 23, 2024
b34c360
fix: revert 'if (nextCharCode === 58)' stmt to one line
jean-michelet Feb 23, 2024
9bb5967
fix: remove useless parametric node kind check
jean-michelet Feb 24, 2024
26b6214
fix: if it is a wildchar node, it has a child
jean-michelet Feb 24, 2024
8a86282
fix: a wildcard route created via the router api necessarily has a pa…
jean-michelet Feb 24, 2024
81c1e12
fix: non-custom strategy error only happen when user dont use the app…
jean-michelet Feb 24, 2024
ed9aa9f
fix: there is no way found route params is not an array, expect if us…
jean-michelet Feb 24, 2024
f028a10
test: Constrainer.noteUsage
jean-michelet Feb 24, 2024
84c1541
Cannot derive constraints without active strategies.
jean-michelet Feb 24, 2024
3467d7c
test: should derive multiple async constraints
jean-michelet Feb 24, 2024
03bd828
test: Major version must be a numeric value
jean-michelet Feb 24, 2024
d9decb9
test: SemVerStore.maxMajor should increase automatically
jean-michelet Feb 24, 2024
3c3785d
test: SemVerStore.maxPatches should increase automatically
jean-michelet Feb 24, 2024
96c367b
refactor: httpMethods module should be a source of truth
jean-michelet Feb 24, 2024
3ba9e4b
refactor: make consistent use of fmw.on
jean-michelet Feb 24, 2024
8788748
fix: istanbul should ignore the complete function
jean-michelet Feb 25, 2024
7cc5bb0
test: remove 90 min limit for code coverage
jean-michelet Feb 25, 2024
6e6f03e
fix: when no args is passed, undefined is default value
jean-michelet Feb 25, 2024
1c29f91
fix: add new line at the end of .taprc
jean-michelet Feb 25, 2024
4370ccb
fix: readd removed comment indicating static route
jean-michelet Feb 26, 2024
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
6 changes: 1 addition & 5 deletions .taprc
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
ts: false
jsx: false
flow: false
branches: 90
functions: 90
lines: 90
statements: 90
flow: false
61 changes: 25 additions & 36 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,6 @@ Router.prototype.findRoute = function findNode (method, path, constraints = {})
const isRegexParam = charCode === 40
const isStaticPart = charCode === 45 || charCode === 46
const isEndOfNode = charCode === 47 || j === pattern.length

if (isRegexParam || isStaticPart || isEndOfNode) {
const paramName = pattern.slice(lastParamStartIndex, j)
params.push(paramName)
Expand Down Expand Up @@ -407,9 +406,7 @@ Router.prototype.findRoute = function findNode (method, path, constraints = {})
// add the wildcard parameter
params.push('*')
currentNode = currentNode.getWildcardChild()
if (currentNode === null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncovered branch.

Can't find a good reason why this could be null using apis.

return null
}

parentNodePathIndex = i + 1

if (i !== pattern.length - 1) {
Expand All @@ -422,10 +419,6 @@ Router.prototype.findRoute = function findNode (method, path, constraints = {})
pattern = pattern.toLowerCase()
}

if (pattern === '*') {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncovered branch.

Can't find a good reason why pattern could be * using apis.

pattern = '/*'
}

for (const existRoute of this.routes) {
const routeConstraints = existRoute.opts.constraints || {}
if (
Expand All @@ -436,7 +429,7 @@ Router.prototype.findRoute = function findNode (method, path, constraints = {})
return {
handler: existRoute.handler,
store: existRoute.store,
params: existRoute.params || []
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncovered branch.

route.params is assigned as an array and never reassigned, do I miss something?

params: existRoute.params
}
}
}
Expand Down Expand Up @@ -625,7 +618,6 @@ Router.prototype.find = function find (method, path, derivedConstraints) {

currentNode = node

// static route
mcollina marked this conversation as resolved.
Show resolved Hide resolved
if (currentNode.kind === NODE_TYPES.STATIC) {
pathIndex += currentNode.prefix.length
continue
Expand All @@ -642,37 +634,36 @@ Router.prototype.find = function find (method, path, derivedConstraints) {
continue
}

if (currentNode.kind === NODE_TYPES.PARAMETRIC) {
let paramEndIndex = originPath.indexOf('/', pathIndex)
if (paramEndIndex === -1) {
paramEndIndex = pathLen
}
// parametric node
let paramEndIndex = originPath.indexOf('/', pathIndex)
if (paramEndIndex === -1) {
paramEndIndex = pathLen
}

let param = originPath.slice(pathIndex, paramEndIndex)
if (shouldDecodeParam) {
param = safeDecodeURIComponent(param)
}
let param = originPath.slice(pathIndex, paramEndIndex)
if (shouldDecodeParam) {
param = safeDecodeURIComponent(param)
}

if (currentNode.isRegex) {
const matchedParameters = currentNode.regex.exec(param)
if (matchedParameters === null) continue
if (currentNode.isRegex) {
const matchedParameters = currentNode.regex.exec(param)
if (matchedParameters === null) continue

for (let i = 1; i < matchedParameters.length; i++) {
const matchedParam = matchedParameters[i]
if (matchedParam.length > maxParamLength) {
return null
}
params.push(matchedParam)
}
} else {
if (param.length > maxParamLength) {
for (let i = 1; i < matchedParameters.length; i++) {
const matchedParam = matchedParameters[i]
if (matchedParam.length > maxParamLength) {
return null
}
params.push(param)
params.push(matchedParam)
}

pathIndex = paramEndIndex
} else {
if (param.length > maxParamLength) {
return null
}
params.push(param)
}

pathIndex = paramEndIndex
}
}

Expand Down Expand Up @@ -742,8 +733,6 @@ for (const i in httpMethods) {
const m = httpMethods[i]
const methodName = m.toLowerCase()

if (Router.prototype[methodName]) throw new Error('Method already exists: ' + methodName)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncovered branch.

I think httpMethods should be a trusted source: if it contains duplicates, the tests will fail in one way or another.


Router.prototype[methodName] = function (path, handler, store) {
return this.on(m, path, handler, store)
}
Expand Down
4 changes: 1 addition & 3 deletions lib/constrainer.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,10 +153,8 @@ class Constrainer {
if (!strategy.isCustom) {
if (key === 'version') {
lines.push(' version: req.headers[\'accept-version\'],')
} else if (key === 'host') {
lines.push(' host: req.headers.host || req.headers[\':authority\'],')
} else {
throw new Error('unknown non-custom strategy for compiling constraint derivation function')
lines.push(' host: req.headers.host || req.headers[\':authority\'],')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncovered branch.

How can we accidentally inject a builtin strategy or forget to manage it?

}
} else {
lines.push(` ${strategy.name}: this.strategies.${key}.deriveConstraint(req, ctx),`)
Expand Down
5 changes: 1 addition & 4 deletions lib/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,10 +129,7 @@ class StaticNode extends ParentNode {
}

getWildcardChild () {
if (this.wildcardChild) {
return this.wildcardChild
}
return null
return this.wildcardChild
mcollina marked this conversation as resolved.
Show resolved Hide resolved
}

createWildcardChild () {
Expand Down
8 changes: 6 additions & 2 deletions lib/strategies/accept-version.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@ SemVerStore.prototype.set = function (version, store) {
}
let [major, minor, patch] = version.split('.')

major = Number(major) || 0
if (isNaN(major)) {
throw new TypeError('Major version must be a numeric value')
mcollina marked this conversation as resolved.
Show resolved Hide resolved
}

major = Number(major)
minor = Number(minor) || 0
patch = Number(patch) || 0

Expand All @@ -38,7 +42,7 @@ SemVerStore.prototype.set = function (version, store) {
this.store[`${major}.x.x`] = store
}

if (patch >= (this.store[`${major}.${minor}`] || 0)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's an error here, but curiously no one seems to have noticed, right?

if (patch >= (this.maxPatches[`${major}.${minor}`] || 0)) {
this.maxPatches[`${major}.${minor}`] = patch
this.store[`${major}.${minor}.x`] = store
}
Expand Down
5 changes: 1 addition & 4 deletions lib/strategies/http-method.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,6 @@ module.exports = {
set: (type, store) => { handlers[type] = store }
}
},
deriveConstraint: (req) => {
/* istanbul ignore next */
return req.method
},
deriveConstraint: /* istanbul ignore next */ (req) => req.method,
mustMatchWhenDerived: true
}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"chalk": "^4.1.2",
"inquirer": "^8.2.4",
"pre-commit": "^1.2.2",
"proxyquire": "^2.1.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw it has a dependency of Fastify, so I tough it could be welcome here.
https://github.com/fastify/fastify/blob/7e50e5307c9a6593a2e43171912630033aa1aacb/package.json#L189

"rfdc": "^1.3.0",
"simple-git": "^3.7.1",
"standard": "^14.3.4",
Expand Down
10 changes: 7 additions & 3 deletions test/constraint.custom.async.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
const t = require('tap')
const test = t.test
const FindMyWay = require('..')
const rfdc = require('rfdc')({ proto: true })

const customHeaderConstraint = {
name: 'requestedBy',
Expand All @@ -23,11 +24,14 @@ const customHeaderConstraint = {
}
}

test('should derive async constraint', t => {
test('should derive multiple async constraints', t => {
t.plan(2)

const router = FindMyWay({ constraints: { requestedBy: customHeaderConstraint } })
router.on('GET', '/', { constraints: { requestedBy: 'node' } }, () => 'asyncHandler')
const customHeaderConstraint2 = rfdc(customHeaderConstraint)
customHeaderConstraint2.name = 'requestedBy2'

const router = FindMyWay({ constraints: { requestedBy: customHeaderConstraint, requestedBy2: customHeaderConstraint2 } })
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To evaluate this branch:

if (--asyncConstraintsCount === 0) {

router.on('GET', '/', { constraints: { requestedBy: 'node', requestedBy2: 'node' } }, () => 'asyncHandler')

router.lookup(
{
Expand Down
Loading