-
Notifications
You must be signed in to change notification settings - Fork 154
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
Bunch of tests reproducing #226 and #227 #228
base: master
Are you sure you want to change the base?
Conversation
Also yeah typically it is more useful to have separate PRs that contain only the changes necessary to repro the issue but just having a repro at all is helpful. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like supertest swallows some errors, so I'd make sure the tests fail when the assertions fail.
.get('/foo') | ||
.expect(200) | ||
.expect(function (res) { | ||
assert.equal( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be better as assert.ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have enough context to validate the problems being tested for, however I highlighted a few spots where superTest
wont fail. It looks like a few places where the suite would return strings in stead of throwing errors got copied into your tests. Returning strings doesn't fail the test.
I mistakenly assumed this was due to using expect
twice in a row. That's fine to do - however in order for it to pass errors to done, an Error()
object must be thrown.
if (!(typeof a === "object" && a.b === "c")) { | ||
return "Default not shown"; | ||
if (!(typeof a === 'object' && a.b === 'c')) { | ||
return 'Default not shown'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be throw new Error('Default not shown');
for super test to pass it to done.
@@ -215,7 +228,7 @@ describe('modes', function () { | |||
.expect(404) | |||
.expect(function (res) { | |||
if (JSON.parse(res.text).error !== 'not_found') { | |||
return "Wrong response body"; | |||
return 'Wrong response body'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must throw an error
); | ||
// console.log(res.text); | ||
if (!/<titl>PouchDB Server<\/title>/.test(res.text)) { | ||
return "No '<title>PouchDB Server</title>' in response"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must throw an Error
|
||
new InMemPouchDB('foo'); | ||
|
||
request(app) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be a little cleaner if you return request(app)
's promise chain instead of using done()
Sorry it's quite messy but at least it reproduces those 2 issues on my machine. There's a lot of cruft since I'm having other problems I can't quite wrap my head around, but maybe this will help track down those first two problems.