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

Ensure test failures #231

Merged
merged 2 commits into from
Jun 19, 2017
Merged

Ensure test failures #231

merged 2 commits into from
Jun 19, 2017

Conversation

reconbot
Copy link
Contributor

@reconbot reconbot commented Jun 9, 2017

The double expect and the return instead of throw wasn't working.

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

looks good to me but would like another review to make sure I’m not missing anything

@gr2m gr2m requested a review from garrensmith June 9, 2017 10:27
@reconbot
Copy link
Contributor Author

reconbot commented Jun 11, 2017

I did some more research and testing with superTest and figure out that it's promise compatible in the sense that you can call then instead of end. I also saw that I was throwing an incorrect message.

@reconbot
Copy link
Contributor Author

The superTest expect(function() {}) doesn't pass non errors to done(). Returning strings, and throwing strings don't work. I tested each modified test to see if it would really fail and they wouldn't. But with these changes, now they do.

@reconbot reconbot changed the title Ensure test failure when fauxton doesn't load Ensure test failures Jun 11, 2017
@gr2m
Copy link
Contributor

gr2m commented Jun 15, 2017

@reconbot any reason why we wouldn’t adapt the same convention for all tests that use supertest?

@reconbot
Copy link
Contributor Author

We should do just that - I only addressed the unit tests in this PR

@gr2m
Copy link
Contributor

gr2m commented Jun 15, 2017

we only use tests/express-pouchdb/test.js right now. But we can make it in a follow up PR, too.

It’s been open for a week now, I’d say we go ahead and merge it in if you are ready from your side?

Copy link
Member

@garrensmith garrensmith left a comment

Choose a reason for hiding this comment

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

Great thanks.

@garrensmith garrensmith merged commit 445e42e into pouchdb:master Jun 19, 2017
@reconbot reconbot deleted the fix-test branch July 9, 2017 23:38
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.

3 participants