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

fix: types and tests coverage #110

Merged
merged 16 commits into from
Sep 26, 2021
Merged

Conversation

darkgl0w
Copy link
Member

@darkgl0w darkgl0w commented Sep 25, 2021

Hello,

closes issue #102 and closes #103 that seems to be abandoned.

  • initialize fastify.redis decorator with Object.create(null)
  • add test cases to improve coverage
  • add namespaced instance types
  • typescript namings have been changed to be more explicit (deprecate FastifyRedisPlugin and use FastifyRedisPluginOptions instead)
  • add typescript tests
  • add a few commands to make maintenance easier on this package
  • achieve 100% coverage (closes Coverage 100% #84)

Checklist

index.d.ts Show resolved Hide resolved
Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

LGTM.

@darkgl0w
Copy link
Member Author

Currently there are only 2 statements that are never tested (current coverage is 95.83%):

  • the index.js catch block line 31
  • the index.js catch block line 53

Maybe I am just too tired but my guesses are that with the way errors are handled those paths are impossible to reach and I don't think there should be try...catch blocks here... WDYT @climba03003 ?

@climba03003
Copy link
Member

climba03003 commented Sep 25, 2021

index.js#L31,L53 is a rare case but it will throw when connection error occur.
https://github.com/luin/ioredis/blob/f6d7777c80a59c528598d94033a2ab8d16b9b6e6/lib/redis/index.ts#L166
https://github.com/luin/ioredis/blob/f6d7777c80a59c528598d94033a2ab8d16b9b6e6/lib/connectors/StandaloneConnector.ts#L64-L89

In this case, use t.mock or proxyquire to replace ioredis class and throw directly in constructor.

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

Some minor problem found after revisit the test case.

test/test.js Outdated Show resolved Hide resolved
test/test.js Outdated Show resolved Hide resolved
darkgl0w and others added 2 commits September 25, 2021 15:20
Co-authored-by: KaKa <climba03003@gmail.com>
Co-authored-by: KaKa <climba03003@gmail.com>
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@climba03003 climba03003 linked an issue Sep 25, 2021 that may be closed by this pull request
2 tasks
@darkgl0w
Copy link
Member Author

darkgl0w commented Sep 25, 2021

index.js#L31,L53 is a rare case but it will throw when connection error occur.
https://github.com/luin/ioredis/blob/f6d7777c80a59c528598d94033a2ab8d16b9b6e6/lib/redis/index.ts#L166
https://github.com/luin/ioredis/blob/f6d7777c80a59c528598d94033a2ab8d16b9b6e6/lib/connectors/StandaloneConnector.ts#L64-L89

In this case, use t.mock or proxyquire to replace ioredis class and throw directly in constructor.

Thanks to you I found a way to force Redis to actually throw an error that we can catch in order to test those 2 statements ^^

@darkgl0w
Copy link
Member Author

@climba03003 > Concerning the Public exported FastifyRedisPlugin type name WDYT if we make something like this darkgl0w@aae237a in order to enforce explicit types naming without breaking existing users code ?

@climba03003
Copy link
Member

@climba03003 > Concerning the Public exported FastifyRedisPlugin type name WDYT if we make something like this darkgl0w@aae237a in order to enforce explicit types naming without breaking existing users code ?

Yes. it is fine.

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

LGTM.

@climba03003 climba03003 merged commit 089deba into fastify:master Sep 26, 2021
@darkgl0w darkgl0w deleted the fix-types-coverage branch September 26, 2021 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to register multiple Redis client instances with Typescript Coverage 100%
3 participants