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

Add promise support for sessionStore functions #635

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

brandonmanke
Copy link

@brandonmanke brandonmanke commented Feb 24, 2019

Potential solution to #607.

I can add some more tests to improve coverage if necessary.

@brandonmanke
Copy link
Author

@dougwilson I can add the should error without callback test for .reload() if that is ok? Also how would you recommend explicitly testing for errors with and without callbacks? Since the tests themselves will be almost identical.

@dougwilson
Copy link
Contributor

Hey, so I added a couple more conditions on Monday to further bump the coverage to help out 👍Feel free to add some new ones as well; I just pushed up without the rebase on Monday just in case you were working on some as well.

I took a look at the coveralls config and it's set only to care about line coverage. So I wouldn't really worry about the reject branches to get it landed. If you want to add them anyway as an exercise, I would just copy-paste the tests at this time.

@brandonmanke
Copy link
Author

How would you recommend testing https://coveralls.io/builds/21919265/source?filename=index.js#L385?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants