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

Added expectBodyLength function for all content types #116

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

Added expectBodyLength function for all content types #116

wants to merge 4 commits into from

Conversation

gwcgm1
Copy link

@gwcgm1 gwcgm1 commented Jun 19, 2018

Had a go at adding this function - found a use for it ourselves but might not be so useful to others.

@Fishbowler
Copy link
Collaborator

Thanks for the contribution @gwcgm1!
Any chance you could add some unit tests for this?
There's some good examples of passing/failing expects based on body around here: https://github.com/IcedFrisby/IcedFrisby/blob/master/test/frisby_mock_request.js#L1135

@Fishbowler
Copy link
Collaborator

I've been looking at some similar missing coverage in Coveralls, and I'm starting to believe that the npm request package used under the hood guarantees body to never be null once you're running expects on the response.

The only way I can see to exercise that null check / error throwing line would be to set a mock in the params, and have that mock function return a null body - a faff too far for the scope of this.

@Fishbowler
Copy link
Collaborator

I've removed the need for this check in the unit-test-coverage branch. Definitely don't worry about it. In fact, you can remove the entire isUndefined check - as I said, it can't occur without mocking.

@Fishbowler
Copy link
Collaborator

I've removed that check (included in PR #124, merging soon), and had a punt at some unit tests.
I'm convinced of coverage, but might be a touch extreme, and might need clearer wording too!

frisby.create('Ensure this is *actually* a real teapot, not some imposter coffee pot')
.get('http://httpbin.org/status/418')
.expectStatus(418)
.expectBodyLength(418,'>')
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the operator is required, would it be a little clearer to put it before the number? Then this reads like an expression: "expect body length greater than 418".

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.

None yet

3 participants