-
Notifications
You must be signed in to change notification settings - Fork 116
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
Return 'null' on status 404 #57
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,7 @@ describe('Rest Tests', function () { | |
|
||
it('constructs', () => { | ||
this.timeout(1000); | ||
|
||
let rest: restm.RestClient = new restm.RestClient('typed-test-client-tests'); | ||
assert(rest, 'rest client should not be null'); | ||
}) | ||
|
@@ -38,75 +38,75 @@ describe('Rest Tests', function () { | |
|
||
let restRes: restm.IRestResponse<HttpBinData> = await _rest.get<HttpBinData>('https://httpbin.org/get'); | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
assert(restRes.result.url === 'https://httpbin.org/get'); | ||
assert(restRes.result && restRes.result.url === 'https://httpbin.org/get'); | ||
}); | ||
|
||
it('gets a resource with baseUrl', async() => { | ||
let restRes: restm.IRestResponse<HttpBinData> = await _restBin.get<HttpBinData>('get'); | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
assert(restRes.result.url === 'https://httpbin.org/get'); | ||
assert(restRes.result && restRes.result.url === 'https://httpbin.org/get'); | ||
}); | ||
|
||
it('creates a resource', async() => { | ||
let res: any = { name: 'foo' }; | ||
let restRes: restm.IRestResponse<HttpBinData> = await _rest.create<HttpBinData>('https://httpbin.org/post', res); | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
assert(restRes.result.url === 'https://httpbin.org/post'); | ||
assert(restRes.result.json.name === 'foo'); | ||
assert(restRes.result && restRes.result.url === 'https://httpbin.org/post'); | ||
assert(restRes.result && restRes.result.json.name === 'foo'); | ||
}); | ||
|
||
it('creates a resource with a baseUrl', async() => { | ||
let res: any = { name: 'foo' }; | ||
let restRes: restm.IRestResponse<HttpBinData> = await _restBin.create<HttpBinData>('post', res); | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
assert(restRes.result.url === 'https://httpbin.org/post'); | ||
assert(restRes.result.json.name === 'foo'); | ||
}); | ||
assert(restRes.result && restRes.result.url === 'https://httpbin.org/post'); | ||
assert(restRes.result && restRes.result.json.name === 'foo'); | ||
}); | ||
|
||
it('replaces a resource', async() => { | ||
let res: any = { name: 'foo' }; | ||
let restRes: restm.IRestResponse<HttpBinData> = await _rest.replace<HttpBinData>('https://httpbin.org/put', res); | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
assert(restRes.result.url === 'https://httpbin.org/put'); | ||
assert(restRes.result.json.name === 'foo'); | ||
assert(restRes.result && restRes.result.url === 'https://httpbin.org/put'); | ||
assert(restRes.result && restRes.result.json.name === 'foo'); | ||
}); | ||
|
||
it('replaces a resource with a baseUrl', async() => { | ||
let res: any = { name: 'foo' }; | ||
let restRes: restm.IRestResponse<HttpBinData> = await _restBin.replace<HttpBinData>('put', res); | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
assert(restRes.result.url === 'https://httpbin.org/put'); | ||
assert(restRes.result.json.name === 'foo'); | ||
assert(restRes.result && restRes.result.url === 'https://httpbin.org/put'); | ||
assert(restRes.result && restRes.result.json.name === 'foo'); | ||
}); | ||
|
||
it('updates a resource', async() => { | ||
let res: any = { name: 'foo' }; | ||
let restRes: restm.IRestResponse<HttpBinData> = await _rest.update<HttpBinData>('https://httpbin.org/patch', res); | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
assert(restRes.result.url === 'https://httpbin.org/patch'); | ||
assert(restRes.result.json.name === 'foo'); | ||
assert(restRes.result && restRes.result.url === 'https://httpbin.org/patch'); | ||
assert(restRes.result && restRes.result.json.name === 'foo'); | ||
}); | ||
|
||
it('updates a resource with a baseUrl', async() => { | ||
let res: any = { name: 'foo' }; | ||
let restRes: restm.IRestResponse<HttpBinData> = await _restBin.update<HttpBinData>('patch', res); | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
assert(restRes.result.url === 'https://httpbin.org/patch'); | ||
assert(restRes.result.json.name === 'foo'); | ||
}); | ||
assert(restRes.result && restRes.result.url === 'https://httpbin.org/patch'); | ||
assert(restRes.result && restRes.result.json.name === 'foo'); | ||
}); | ||
|
||
it('deletes a resource', async() => { | ||
let restRes: restm.IRestResponse<HttpBinData> = await _rest.del<HttpBinData>('https://httpbin.org/delete'); | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
assert(restRes.result.url === 'https://httpbin.org/delete'); | ||
assert(restRes.result && restRes.result.url === 'https://httpbin.org/delete'); | ||
}); | ||
|
||
it('deletes a resource with a baseUrl', async() => { | ||
let restRes: restm.IRestResponse<HttpBinData> = await _restBin.del<HttpBinData>('delete'); | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
assert(restRes.result.url === 'https://httpbin.org/delete'); | ||
}); | ||
assert(restRes.result && restRes.result.url === 'https://httpbin.org/delete'); | ||
}); | ||
|
||
it('does an options request', async() => { | ||
let restRes: restm.IRestResponse<HttpBinData> = await _rest.options<HttpBinData>('https://httpbin.org'); | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
|
@@ -115,7 +115,7 @@ describe('Rest Tests', function () { | |
it('does an options request with baseUrl', async() => { | ||
let restRes: restm.IRestResponse<HttpBinData> = await _restBin.options<HttpBinData>(''); | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
}); | ||
}); | ||
|
||
//---------------------------------------------- | ||
// Get Error Cases | ||
|
@@ -128,9 +128,9 @@ describe('Rest Tests', function () { | |
it('gets a non-existant resource (404)', async() => { | ||
try { | ||
let restRes: restm.IRestResponse<HttpBinData> = await _rest.get<HttpBinData>('https://httpbin.org/status/404'); | ||
|
||
assert(restRes.statusCode == 404, "statusCode should be 404"); | ||
assert(restRes.result == null, "object should be null"); | ||
assert(restRes.result === null, "object should be null"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. result object should be null? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to your documentation in https://github.com/Microsoft/typed-rest-client/blob/master/README.md a 404 should return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep - in the REST layer since it's job is to return a restful object, a 404 should return a null. However the http layer will return body. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'm fine with these lines / changes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did a poor job of writing that comment. I meant that the string could be changed to be more expressive. |
||
} | ||
catch(err) { | ||
assert(false, "should not throw"); | ||
|
@@ -141,7 +141,7 @@ describe('Rest Tests', function () { | |
// Unauthorized (401) | ||
// should throw and attach statusCode to the Error object | ||
// err.message is message proerty of resourceful error object or if not supplied, a generic error message | ||
// | ||
// | ||
it('gets and handles unauthorized (401)', async() => { | ||
try { | ||
let restRes: restm.IRestResponse<HttpBinData> = await _rest.get<HttpBinData>('https://httpbin.org/status/401'); | ||
|
@@ -151,13 +151,13 @@ describe('Rest Tests', function () { | |
assert(err['statusCode'] == 401, "statusCode should be 401"); | ||
assert(err.message && err.message.length > 0, "should have error message"); | ||
} | ||
}); | ||
}); | ||
|
||
// | ||
// Internal Server Error | ||
// should throw and attach statusCode to the Error object | ||
// err.message is message proerty of resourceful error object or if not supplied, a generic error message | ||
// | ||
// | ||
it('gets and handles a server error (500)', async() => { | ||
try { | ||
let restRes: restm.IRestResponse<HttpBinData> = await _rest.get<HttpBinData>('https://httpbin.org/status/500'); | ||
|
@@ -183,7 +183,7 @@ describe('Rest Tests', function () { | |
|
||
// Assert | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
assert(restRes.result.url === 'https://httpbin.org/anything/anythingextra'); | ||
assert(restRes.result && restRes.result.url === 'https://httpbin.org/anything/anythingextra'); | ||
}); | ||
|
||
it('maintains the path from the base url with no slashes', async() => { | ||
|
@@ -195,7 +195,7 @@ describe('Rest Tests', function () { | |
|
||
// Assert | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
assert(restRes.result.url === 'https://httpbin.org/anything/anythingextra'); | ||
assert(restRes.result && restRes.result.url === 'https://httpbin.org/anything/anythingextra'); | ||
}); | ||
|
||
it('maintains the path from the base url with double slashes', async() => { | ||
|
@@ -207,7 +207,7 @@ describe('Rest Tests', function () { | |
|
||
// Assert | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
assert(restRes.result.url === 'https://httpbin.org/anything/anythingextra'); | ||
assert(restRes.result && restRes.result.url === 'https://httpbin.org/anything/anythingextra'); | ||
}); | ||
|
||
it('maintains the path from the base url with multiple parts', async() => { | ||
|
@@ -219,7 +219,7 @@ describe('Rest Tests', function () { | |
|
||
// Assert | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
assert(restRes.result.url === 'https://httpbin.org/anything/extrapart/anythingextra'); | ||
assert(restRes.result && restRes.result.url === 'https://httpbin.org/anything/extrapart/anythingextra'); | ||
}); | ||
|
||
it('maintains the path from the base url where request has multiple parts', async() => { | ||
|
@@ -231,7 +231,7 @@ describe('Rest Tests', function () { | |
|
||
// Assert | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
assert(restRes.result.url === 'https://httpbin.org/anything/anythingextra/moreparts'); | ||
assert(restRes.result && restRes.result.url === 'https://httpbin.org/anything/anythingextra/moreparts'); | ||
}); | ||
|
||
it('maintains the path from the base url where both have multiple parts', async() => { | ||
|
@@ -243,7 +243,7 @@ describe('Rest Tests', function () { | |
|
||
// Assert | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
assert(restRes.result.url === 'https://httpbin.org/anything/multiple/anythingextra/moreparts'); | ||
assert(restRes.result && restRes.result.url === 'https://httpbin.org/anything/multiple/anythingextra/moreparts'); | ||
}); | ||
|
||
it('maintains the path from the base url where request has query parameters', async() => { | ||
|
@@ -255,9 +255,9 @@ describe('Rest Tests', function () { | |
|
||
// Assert | ||
assert(restRes.statusCode == 200, "statusCode should be 200"); | ||
assert(restRes.result.url === 'https://httpbin.org/anything/multiple/anythingextra/moreparts?foo=bar&baz=top'); | ||
assert(restRes.result.args.foo === 'bar'); | ||
assert(restRes.result.args.baz === 'top'); | ||
assert(restRes.result && restRes.result.url === 'https://httpbin.org/anything/multiple/anythingextra/moreparts?foo=bar&baz=top'); | ||
assert(restRes.result && restRes.result.args.foo === 'bar'); | ||
assert(restRes.result && restRes.result.args.baz === 'top'); | ||
}); | ||
|
||
// | ||
|
@@ -272,11 +272,11 @@ describe('Rest Tests', function () { | |
let res: string = util.getUrl('', 'http://httpbin.org'); | ||
assert(res === 'http://httpbin.org', "should be http://httpbin.org"); | ||
}); | ||
|
||
it('resolves a null resource with baseUrl', async() => { | ||
let res: string = util.getUrl(null, 'http://httpbin.org'); | ||
assert(res === 'http://httpbin.org', "should be http://httpbin.org"); | ||
}); | ||
}); | ||
|
||
it('resolves a full resource and no baseUrl', async() => { | ||
let res: string = util.getUrl('http://httpbin.org/get?x=y&a=b'); | ||
|
@@ -291,7 +291,7 @@ describe('Rest Tests', function () { | |
it('resolves a relative path resource with host baseUrl', async() => { | ||
let res: string = util.getUrl('get/foo', 'http://httpbin.org'); | ||
assert(res === 'http://httpbin.org/get/foo', `should be http://httpbin.org/get/foo but is ${res}`); | ||
}); | ||
}); | ||
|
||
it('resolves a rooted path resource with pathed baseUrl', async() => { | ||
let res: string = util.getUrl('/get/foo', 'http://httpbin.org/bar'); | ||
|
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.
For all of these, I am not sure it adds value to have the restRes.result check before doing an equality check. It fails either way and since this is just a test I don't think we need it. Makes it harder to read too.
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.
Same comment for all places this has been added.
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 added these checks because I usually compile code with "strict" type checking enabled. If this option is disabled, then it would produces no error message, but in my eye, it is actually semantically wrong.