Achieve 100% test coverage #349

jean-michelet · 2024-02-22T18:02:38Z

Hi, first time contributor here 👋

Will fixes #330

So far, I've only worked on one test file, but I think I should try to improve existing tests or create new isolated test files.

I don't have much confidence in the tests I've written, so I'd like to get some initial feedback to make sure I'm on the right track.

Although all filenames are green now, yellow has gradually appeared...
I'm not familiar with test coverage tools but this seems to be related to conditional statements and logical expressions, right?
How important is it for you to cover these cases? To what extent?

test: findMyWay.findRoute should also trows an error when wildcard is not the last character test: does not find the route if maxParamLength is exceeded refactor: make integration test more compliant with the existing code base test: cover uncovered constrainer.js test cases test: safeDecodeURIComponent should replace %3x to null for every x that is not a valid lowchar test: SemVerStore version should be a string test: httpMethodStrategy storage handles set and get operations correctly test: case insensitive static routes of level 1 for FindMyWay.findRoute test: findRoute normalizes wildcard patterns to require leading slash refactor: make tests more consistants with the existing test: should return null if no wildchar child Date: Tue Feb 20 14:10:18 2024 +0100 Achieves 100% test coverage

mcollina · 2024-02-23T09:21:24Z

Although all filenames are green now, yellow has gradually appeared...
I'm not familiar with test coverage tools but this seems to be related to conditional statements and logical expressions, right?
How important is it for you to cover these cases? To what extent?

You should make them all green. They are yellow because the lines are covered, but the branches of those conditions are not.
Take a look at https://en.wikipedia.org/wiki/Code_coverage.

jean-michelet · 2024-02-23T17:08:32Z

Still have some work to do... But maybe someone can answers these questions.

Is it ok to test directly internal components?

test('throws error if pass an undefined constraint value', (t) => {
  t.plan(1)

  const constrainer = new Constrainer()
  const error = new Error('Can\'t pass an undefined constraint value, must pass null or no key at all')
  t.throws(() => constrainer.validateConstraints({ key: undefined }), error)
})

Or do I have to test through apis?

It seems that there are many preventive conditions that can't be evaluated trough the apis, but can be triggered by modifying the internal state. Here, I'm struggling to find a reason why a route with a wildcard path would have a pattern without a leading slash... Is the condition relevant if there is no way to trigger it through apis?

test('findRoute normalizes wildcard patterns with leading slash', (t) => {
  t.plan(4)

  const findMyWay = FindMyWay()
  findMyWay.get('*', () => {})

  t.equal(findMyWay.routes.length, 1)
  // will match with leading slash
  t.equal(findMyWay.routes[0].pattern, '/*')

  t.ok(findMyWay.findRoute('GET', '*'))

  // will fail if we remove it
  findMyWay.routes[0].pattern = '*'

  t.equal(findMyWay.findRoute('GET', '*'), null)
})

Should I keep the tests in the current test file issue-330.test.js or should I try to distribute them in a relevant way in the existing tests?

…ttern with a leading slash

…ropriate api

…er break internals

mcollina · 2024-02-24T09:57:03Z

better through public APIs, but still ok to call internal methods to achieve code coverage
it really depends on a case-by-case basis. If something can't be evaluated, you might be able to remove it.
ideally keep the tests where they are, but it's ok to refactor things around.

jean-michelet

I feel I can test better via the APIs, I'll look at that on Monday.

This PR is a lot of fun, although technical...

jean-michelet · 2024-02-24T17:30:19Z

index.js

@@ -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)


Uncovered branch.

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

jean-michelet · 2024-02-24T17:33:43Z

lib/constrainer.js

        } else {
-          throw new Error('unknown non-custom strategy for compiling constraint derivation function')
+          lines.push('   host: req.headers.host || req.headers[\':authority\'],')


Uncovered branch.

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

jean-michelet · 2024-02-24T17:35:37Z

index.js

@@ -407,9 +406,7 @@ Router.prototype.findRoute = function findNode (method, path, constraints = {})
      // add the wildcard parameter
      params.push('*')
      currentNode = currentNode.getWildcardChild()
-      if (currentNode === null) {


Uncovered branch.

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

jean-michelet · 2024-02-24T17:37:36Z

index.js

@@ -422,10 +419,6 @@ Router.prototype.findRoute = function findNode (method, path, constraints = {})
    pattern = pattern.toLowerCase()
  }

-  if (pattern === '*') {


Uncovered branch.

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

jean-michelet · 2024-02-24T17:39:38Z

index.js

@@ -436,7 +429,7 @@ Router.prototype.findRoute = function findNode (method, path, constraints = {})
      return {
        handler: existRoute.handler,
        store: existRoute.store,
-        params: existRoute.params || []


Uncovered branch.

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

lib/node.js

jean-michelet · 2024-02-24T17:44:56Z

lib/strategies/accept-version.js

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

-  if (patch >= (this.store[`${major}.${minor}`] || 0)) {


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

jean-michelet · 2024-02-24T17:47:30Z

package.json

@@ -40,6 +40,7 @@
    "chalk": "^4.1.2",
    "inquirer": "^8.2.4",
    "pre-commit": "^1.2.2",
+    "proxyquire": "^2.1.3",


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

jean-michelet · 2024-02-24T17:49:43Z

test/constraint.custom.async.test.js

+  const customHeaderConstraint2 = rfdc(customHeaderConstraint)
+  customHeaderConstraint2.name = 'requestedBy2'
+
+  const router = FindMyWay({ constraints: { requestedBy: customHeaderConstraint, requestedBy2: customHeaderConstraint2 } })


To evaluate this branch:

find-my-way/lib/constrainer.js

Line 135 in c4ffd9e

if (--asyncConstraintsCount === 0) {

lib/strategies/accept-version.js

mcollina · 2024-02-25T08:52:29Z

once you are done, remove the last 4 lines from https://github.com/delvedor/find-my-way/blob/main/.taprc. In that way it will fail the test suit if the coverage falls again.

jean-michelet · 2024-02-25T16:32:59Z

100% reached, minimal limits removed.

ivan-tymoshenko

Thanks. I'm a bit worried about the changes in the router's behaviour. I will take a deep look into them next week.

jean-michelet · 2024-02-25T16:40:36Z

Thanks. I'm a bit worried about the changes in the router's behaviour. I will take a deep look into them next week.

I'd also appreciate feedback on how you'd test these lines if you prefer I reintroduce them.

mcollina

lgtm with a nit

index.js

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [find-my-way](https://togithub.com/delvedor/find-my-way) | [`8.1.0` -> `8.2.0`](https://renovatebot.com/diffs/npm/find-my-way/8.1.0/8.2.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/find-my-way/8.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/find-my-way/8.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/find-my-way/8.1.0/8.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/find-my-way/8.1.0/8.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>delvedor/find-my-way (find-my-way)</summary> ### [`v8.2.0`](https://togithub.com/delvedor/find-my-way/releases/tag/v8.2.0) [Compare Source](https://togithub.com/delvedor/find-my-way/compare/v8.1.0...v8.2.0) #### What's Changed - Update contraint check to throw with 32 handlers by [@valeneiko](https://togithub.com/valeneiko) in [delvedor/find-my-way#344 - feat: add node: prefix for assert by [@SavaCool122](https://togithub.com/SavaCool122) in [delvedor/find-my-way#347 - add dependabot.yml by [@Uzlopak](https://togithub.com/Uzlopak) in [delvedor/find-my-way#350 - chore: bump actions/setup-node from 3 to 4 by [@dependabot](https://togithub.com/dependabot) in [delvedor/find-my-way#351 - chore: bump actions/checkout from 3 to 4 by [@dependabot](https://togithub.com/dependabot) in [delvedor/find-my-way#352 - Achieve 100% test coverage by [@jean-michelet](https://togithub.com/jean-michelet) in [delvedor/find-my-way#349 - chore: bump the dependencies-major group with 1 update by [@dependabot](https://togithub.com/dependabot) in [delvedor/find-my-way#353 - Fix header in README by [@selimb](https://togithub.com/selimb) in [delvedor/find-my-way#345 - Exclude Node v14 and v16 on macos by [@mcollina](https://togithub.com/mcollina) in [delvedor/find-my-way#364 - add node v22. Skip old nodes on mac by [@mcollina](https://togithub.com/mcollina) in [delvedor/find-my-way#363 - Support optional params on root by [@mcollina](https://togithub.com/mcollina) in [delvedor/find-my-way#367 #### New Contributors - [@valeneiko](https://togithub.com/valeneiko) made their first contribution in [delvedor/find-my-way#344 - [@SavaCool122](https://togithub.com/SavaCool122) made their first contribution in [delvedor/find-my-way#347 - [@dependabot](https://togithub.com/dependabot) made their first contribution in [delvedor/find-my-way#351 - [@jean-michelet](https://togithub.com/jean-michelet) made their first contribution in [delvedor/find-my-way#349 - [@selimb](https://togithub.com/selimb) made their first contribution in [delvedor/find-my-way#345 **Full Changelog**: delvedor/find-my-way@v8.1.0...v8.2.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/redwoodjs/redwood).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

jean-michelet added 2 commits February 22, 2024 18:37

fix: add proxyquire to dev dependencies

0ab4156

test: cover uncovered branches in index.js

c9ccb25

jean-michelet added 7 commits February 23, 2024 18:45

fix: revert 'if (nextCharCode === 58)' stmt to one line

b34c360

fix: remove useless parametric node kind check

9bb5967

fix: if it is a wildchar node, it has a child

26b6214

fix: a wildcard route created via the router api necessarily has a pa…

8a86282

…ttern with a leading slash

fix: non-custom strategy error only happen when user dont use the app…

81c1e12

…ropriate api

fix: there is no way found route params is not an array, expect if us…

ed9aa9f

…er break internals

test: Constrainer.noteUsage

f028a10

jean-michelet added 7 commits February 24, 2024 11:20

Cannot derive constraints without active strategies.

84c1541

test: should derive multiple async constraints

3467d7c

test: Major version must be a numeric value

03bd828

test: SemVerStore.maxMajor should increase automatically

d9decb9

test: SemVerStore.maxPatches should increase automatically

3c3785d

refactor: httpMethods module should be a source of truth

96c367b

refactor: make consistent use of fmw.on

3ba9e4b

jean-michelet commented Feb 24, 2024

View reviewed changes

jean-michelet added 3 commits February 25, 2024 17:28

fix: istanbul should ignore the complete function

8788748

test: remove 90 min limit for code coverage

7cc5bb0

fix: when no args is passed, undefined is default value

6e6f03e

ivan-tymoshenko reviewed Feb 25, 2024

View reviewed changes

fix: add new line at the end of .taprc

1c29f91

mcollina approved these changes Feb 25, 2024

View reviewed changes

index.js Outdated Show resolved Hide resolved

fix: readd removed comment indicating static route

4370ccb

jean-michelet requested a review from ivan-tymoshenko March 2, 2024 11:59

mcollina merged commit e117960 into delvedor:main Mar 5, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Achieve 100% test coverage #349

Achieve 100% test coverage #349

jean-michelet commented Feb 22, 2024

mcollina commented Feb 23, 2024

jean-michelet commented Feb 23, 2024 •

edited

mcollina commented Feb 24, 2024

jean-michelet left a comment

jean-michelet Feb 24, 2024

jean-michelet Feb 24, 2024

jean-michelet Feb 24, 2024

jean-michelet Feb 24, 2024

jean-michelet Feb 24, 2024

jean-michelet Feb 24, 2024

jean-michelet Feb 24, 2024

jean-michelet Feb 24, 2024

mcollina commented Feb 25, 2024

jean-michelet commented Feb 25, 2024 •

edited

ivan-tymoshenko left a comment

jean-michelet commented Feb 25, 2024 •

edited

mcollina left a comment

Achieve 100% test coverage #349

Achieve 100% test coverage #349

Conversation

jean-michelet commented Feb 22, 2024

mcollina commented Feb 23, 2024

jean-michelet commented Feb 23, 2024 • edited

mcollina commented Feb 24, 2024

jean-michelet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Feb 25, 2024

jean-michelet commented Feb 25, 2024 • edited

ivan-tymoshenko left a comment

Choose a reason for hiding this comment

jean-michelet commented Feb 25, 2024 • edited

mcollina left a comment

Choose a reason for hiding this comment

jean-michelet commented Feb 23, 2024 •

edited

jean-michelet commented Feb 25, 2024 •

edited

jean-michelet commented Feb 25, 2024 •

edited