Skip to content

Commit

Permalink
tests: add header missing tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@dougwilson
dougwilson committed May 29, 2014
1 parent e7fad24 commit 5975ce4
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 11 deletions.
13 changes: 10 additions & 3 deletions test/charset.js
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
function accepts(charset) {
return require('../')({
headers: {
'accept-charset': charset || ''
'accept-charset': charset
}
})
}
Expand All @@ -15,9 +15,16 @@ describe('accepts.charsets()', function(){
})
})

describe('when Accept-Charset is not populated', function(){
it('should return an empty array', function(){
describe('when Accept-Charset is not in request', function(){
it('should return *', function(){
var accept = accepts();
accept.charsets().should.eql(['*']);
})
})

describe('when Accept-Charset is empty', function(){
it('should return an empty array', function(){
var accept = accepts('');
accept.charsets().should.eql([]);
})
})
Expand Down
19 changes: 17 additions & 2 deletions test/encoding.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
function accepts(encoding) {
return require('../')({
headers: {
'accept-encoding': encoding || ''
'accept-encoding': encoding
}
})
}
Expand All @@ -17,7 +17,7 @@ describe('accepts.encodings()', function(){
})
})

describe('when Accept-Encoding is not populated', function(){
describe('when Accept-Encoding is not in request', function(){
it('should return identity', function(){
var accept = accepts();
accept.encodings().should.eql(['identity']);
Expand All @@ -31,6 +31,21 @@ describe('accepts.encodings()', function(){
})
})
})

describe('when Accept-Encoding is empty', function(){
it('should return identity', function(){
var accept = accepts('');
accept.encodings().should.eql(['identity']);
accept.encodings('gzip', 'deflate', 'identity').should.equal('identity');
})

describe('when identity is not included', function(){
it('should return false', function(){
var accept = accepts('');
accept.encodings('gzip', 'deflate').should.equal(false);
})
})
})
})

describe('with multiple arguments', function(){
Expand Down
13 changes: 10 additions & 3 deletions test/language.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
function accepts(language) {
return require('../')({
headers: {
'accept-language': language || ''
'accept-language': language
}
})
}
Expand All @@ -16,9 +16,16 @@ describe('accepts.languages()', function(){
})
})

describe('when Accept-Language is not populated', function(){
it('should return an empty array', function(){
describe('when Accept-Language is not in request', function(){
it('should return *', function(){
var accept = accepts();
accept.languages().should.eql(['*']);
})
})

describe('when Accept-Language is empty', function(){
it('should return an empty array', function(){
var accept = accepts('');
accept.languages().should.eql([]);
})
})
Expand Down
13 changes: 10 additions & 3 deletions test/type.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
function accepts(type) {
return require('../')({
headers: {
'accept': type || ''
'accept': type
}
})
}
Expand All @@ -16,9 +16,16 @@ describe('accepts.types()', function(){
})
})

describe('when Accept is not populated', function(){
it('should return []', function(){
describe('when Accept not in request', function(){
it('should return */*', function(){
var accept = accepts();
accept.types().should.eql(['*/*']);
})
})

describe('when Accept is empty', function(){
it('should return []', function(){
var accept = accepts('');
accept.types().should.eql([]);
})
})
Expand Down

10 comments on commit 5975ce4

@jonathanong
Copy link
Member

Choose a reason for hiding this comment

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

hmmm i'm not sure if * is a good idea because it's difficult to be semantic with it. but then again, passing nothing into accepts() is retarded in the first place.

@jonathanong
Copy link
Member

Choose a reason for hiding this comment

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

btw, i would consider this a breaking change =P lol

@dougwilson
Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw, i would consider this a breaking change =P lol

Are you sure? Before it was broken i.r.t RFC 2616. And sadly the guy for Negoiator released it as a patch, and this module wasn't pinning that module, lol

@dougwilson
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm i'm not sure if * is a good idea because it's difficult to be semantic with it. but then again, passing nothing into accepts() is retarded in the first place.

So in these tests, accepts is not the actual library, rather a helper function that builds a fake req.headers. The "passing nothing" is just to the helper function to generate a req that has no Accept header.

@dougwilson
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So just as an FYI, before the changes, if a request came in to a server negotiating with this module and had no Accept header in the request, this library would have caused a 416, which is in violation of the HTTP/1.1 spec, which says the absence of the header means all content types are equally acceptable. Probably no one noticed this because almost all clients just send an Accept header.

@dougwilson
Copy link
Contributor Author

Choose a reason for hiding this comment

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

P.S. @jonathanong I don't seem to be able to successfully send an email to the email you have listed for whatever reason :/

@jonathanong
Copy link
Member

Choose a reason for hiding this comment

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

really? pretty sure no accept header resolves to accept: */*, according to spec.

my only issue is that * doesn't mean anything, but then again neither does []. everythiung should be actual mime types and stuff like image/png.

@dougwilson
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I think you are confusing your own API.

really? pretty sure no accept header resolves to accept: /, according to spec.

that's what it's now doing

really? pretty sure no accept header resolves to accept: /, according to spec.

it's the API Negotiator does, and has always done; calling it like those just returns what the browser has in their header. In this case, the browser has no header, so when you ask for what's in the header, it now gives you */* as if that's what the browser had.

everythiung should be actual mime types and stuff like image/png.

yes, they do when you use the other parts of the API that actually do that :)

@jonathanong
Copy link
Member

Choose a reason for hiding this comment

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

ohhh it's a negotiator thing. lameee

@dougwilson
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea. Basically the API that changed was a part that no one is really using. The real API people would use works just the same, but more correct :)

Please sign in to comment.