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 redis test when redis is not running locally #395

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@mayurkale22
Copy link
Collaborator

mayurkale22 commented Mar 6, 2019

No description provided.

@googlebot googlebot added the cla: yes label Mar 6, 2019

@mayurkale22

This comment has been minimized.

Copy link
Collaborator Author

mayurkale22 commented Mar 6, 2019

@vmarchaud Please review

reject(err);
return;
});
resolve(client);

This comment has been minimized.

@vmarchaud

vmarchaud Mar 6, 2019

Contributor

you should listen for the ready event to be able to resolve the promise i think, otherwise it might resolve once before catching the error.

This comment has been minimized.

@mayurkale22

mayurkale22 Mar 7, 2019

Author Collaborator

Done, PTAL

This comment has been minimized.

@vmarchaud

vmarchaud Mar 7, 2019

Contributor

Fine for me

mayurkale22 added some commits Mar 6, 2019

@mayurkale22 mayurkale22 force-pushed the mayurkale22:fix_redis_local_test branch from e702a3f to dc8df81 Mar 7, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 7, 2019

Codecov Report

Merging #395 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #395      +/-   ##
==========================================
- Coverage   94.79%   94.73%   -0.06%     
==========================================
  Files         136      136              
  Lines        8882     8895      +13     
  Branches      655      655              
==========================================
+ Hits         8420     8427       +7     
- Misses        462      468       +6
Impacted Files Coverage Δ
test/test-redis.ts 92.3% <0%> (-4.81%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa97a9b...dc8df81. Read the comment docs.

@mayurkale22 mayurkale22 requested a review from draffensperger Mar 8, 2019

const client = redis.createClient({url});
client.on('error', (err) => {
reject(err);
return;

This comment has been minimized.

@draffensperger

draffensperger Mar 8, 2019

Contributor

Could you remove this extra return; at the end of the arrow function?

@mayurkale22

This comment has been minimized.

Copy link
Collaborator Author

mayurkale22 commented Mar 14, 2019

@vmarchaud Looks like #410 (comment) is valid in this case also. WDYT? I am ok to close this PR if we are going with that route.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.