increase code coverage to 100% #267

ghost · 2020-07-12T11:20:08Z

Currently, 4 out of 4 files have 100% coverage:

File	% Stmts	% Branch	% Funcs	% Lines
All files	100	100	100	100
dynamic.js	100	100	100	100
index.js	100	100	100	100
routes.js	100	100	100	100
static.js	100	100	100	100

Fixes: #258

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
commit message and code follows Code of conduct

mcollina

Good work, I've left a few nits!

static.js

test/static.js

routes.js

test/static.js

mcollina · 2020-07-12T18:09:54Z

test/static.js

+  const next = () => console.log()
+
+  // eslint-disable-next-line
+  fastify.register(Function(fastifySwagger(fastify, null, next)))


Why the Function?

If we don't use a Function constructor, avvio throws an error saying that:

FAIL test/static.js ✖ plugin must be a function or a promise node_modules/avvio/boot.js 192 | function assertPlugin (plugin) { 193 | if (!(plugin && (typeof plugin === 'function' || typeof plugin.then === 'function'))) { > 194 | throw new Error('plugin must be a function or a promise') | ----------^ 195 | } 196 | } 197 |

So, if we remove the Function for this test, it'll fail

mcollina · 2020-07-12T18:11:15Z

** I have some issues with dynamic.js would be nice if you can help me out with it.**

what issues?

ghost · 2020-07-12T18:38:04Z

** I have some issues with dynamic.js would be nice if you can help me out with it.**

what issues?

Yeah, I've spent a couple of days trying to cover all the remaining lines/statements/branches inside dynamic.js file but I just can't reach them. So, it would be nice to have some new ideas from you or someone else how I can do it.
Additionally, there are 8 uncovered branches (approx. half of them do not have an else block). What should I do with that half? Add some kind of else { console.log() } or we can ignore them?

mcollina · 2020-07-12T18:41:47Z

Which lines are unreacheable? You can paste links to the line in the issue.

Additionally, there are 8 uncovered branches (approx. half of them do not have an else block). What should I do with that half? Add some kind of else { console.log() }

I don't understand why adding an else and a console.log helps. You need to add a test where the main condition in the if is not triggered.

ghost · 2020-07-12T19:16:42Z

Which lines are unreacheable? You can paste links to the line in the issue.

Additionally, there are 8 uncovered branches (approx. half of them do not have an else block). What should I do with that half? Add some kind of else { console.log() }

I don't understand why adding an else and a console.log helps. You need to add a test where the main condition in the if is not triggered.

I'll try more then

dynamic.js

mcollina

Good job! I think you have found a few dead code paths!

dynamic.js

routes.js

mcollina · 2020-07-17T10:20:38Z

how are we for code coverage now?

ghost · 2020-07-17T10:32:42Z

Hey @mcollina

We're approx. at the same point. I'm currently trying to cover the remaining statements and branches :)
Hopefully, I'll push them today or maybe tomorrow

dynamic.js

ghost · 2020-07-17T10:56:09Z

dynamic.js

-    const localReference = jsonSchema.$ref.split('/')[2]
-    return localRefResolve(externalSchemas[localReference], externalSchemas)
-  }
-  return jsonSchema


We don't need this "default" return statement. In this function, the flow goes either into the first if or into the second one due to the schema validation rules as far as I understood.
Therefore, I refactored things a bit.

ghost · 2020-07-17T10:56:31Z

package.json

@@ -5,7 +5,7 @@
  "main": "index.js",
  "scripts": {
    "prepare": "node prepare-swagger-ui",
-    "test": "standard && tap test/*.js && npm run typescript",
+    "test": "standard && tap --100 test/*.js && npm run typescript",


To enforce 100% percent coverage in the future

dynamic.js

ghost · 2020-07-18T10:56:54Z

dynamic.js

@@ -106,22 +106,20 @@ module.exports = function (fastify, opts, next) {
      swaggerObject.externalDocs = externalDocs
    }

-    if (!ref) {


I've played with code a bit more and found out that we don't need this if clause. The swagger function is called only once at the very beginning, and the second call does not allow us to trigger it once again. Therefore, we're quite safe to remove it, it should not break any logic.

mcollina

lgtm, really good work!

ghost · 2020-07-24T10:41:59Z

Hey @mcollina

Should I add something or this can be merged to the main branch? Do I need to request some more reviews?

mcollina · 2020-07-24T11:46:40Z

I was waiting for more reviews.. but not really needed! Landed!

ghost mentioned this pull request Jul 12, 2020

100% code coverage #258

Closed

mcollina reviewed Jul 12, 2020

View reviewed changes

ghost commented Jul 14, 2020

View reviewed changes

dynamic.js Outdated Show resolved Hide resolved

dynamic.js Outdated Show resolved Hide resolved

dynamic.js Outdated Show resolved Hide resolved

ghost changed the title ~~increase code coverage to 98.68%~~ increase code coverage to >99.0% Jul 14, 2020

mcollina reviewed Jul 14, 2020

View reviewed changes

dynamic.js Outdated Show resolved Hide resolved

dynamic.js Outdated Show resolved Hide resolved

dynamic.js Outdated Show resolved Hide resolved

routes.js Outdated Show resolved Hide resolved

mcollina reviewed Jul 17, 2020

View reviewed changes

dynamic.js Outdated Show resolved Hide resolved

dynamic.js Outdated Show resolved Hide resolved

ghost commented Jul 17, 2020

View reviewed changes

increase code coverage

6ceda6a

ghost commented Jul 18, 2020

View reviewed changes

ghost changed the title ~~increase code coverage to >99.0%~~ increase code coverage to 100% Jul 18, 2020

mcollina approved these changes Jul 18, 2020

View reviewed changes

mcollina merged commit adf0c95 into fastify:master Jul 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

increase code coverage to 100% #267

increase code coverage to 100% #267

ghost commented Jul 12, 2020 •

edited by ghost

Loading

mcollina left a comment

mcollina Jul 12, 2020

ghost Jul 12, 2020

mcollina commented Jul 12, 2020

ghost commented Jul 12, 2020

mcollina commented Jul 12, 2020

ghost commented Jul 12, 2020

mcollina left a comment

mcollina commented Jul 17, 2020

ghost commented Jul 17, 2020

ghost Jul 17, 2020

ghost Jul 17, 2020

mcollina Jul 17, 2020

ghost Jul 18, 2020

mcollina left a comment

ghost commented Jul 24, 2020

mcollina commented Jul 24, 2020

increase code coverage to 100% #267

increase code coverage to 100% #267

Conversation

ghost commented Jul 12, 2020 • edited by ghost Loading

Checklist

mcollina left a comment

Choose a reason for hiding this comment

mcollina Jul 12, 2020

Choose a reason for hiding this comment

ghost Jul 12, 2020

Choose a reason for hiding this comment

mcollina commented Jul 12, 2020

ghost commented Jul 12, 2020

mcollina commented Jul 12, 2020

ghost commented Jul 12, 2020

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Jul 17, 2020

ghost commented Jul 17, 2020

ghost Jul 17, 2020

Choose a reason for hiding this comment

ghost Jul 17, 2020

Choose a reason for hiding this comment

mcollina Jul 17, 2020

Choose a reason for hiding this comment

ghost Jul 18, 2020

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

ghost commented Jul 24, 2020

mcollina commented Jul 24, 2020

ghost commented Jul 12, 2020 •

edited by ghost

Loading