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

Resolved Issue #804 #862

Merged
merged 1 commit into from Feb 12, 2018
Merged

Resolved Issue #804 #862

merged 1 commit into from Feb 12, 2018

Conversation

dcstone09
Copy link
Contributor

@dcstone09 dcstone09 commented Mar 22, 2017

Hello, I think this fixes #804. I am not sure where to put my test case exactly. Any feedback would be great thanks.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 92.676% when pulling 96f0c00 on dcstone09:header_conflict into 934f1c3 on node-nock:master.

Copy link
Contributor

@ianwsperber ianwsperber left a comment

Choose a reason for hiding this comment

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

@dcstone09 Thanks for this! Instead of putting your test in a new file, please add to tests/test_intercept.js.

@coveralls
Copy link

coveralls commented Mar 29, 2017

Coverage Status

Coverage increased (+0.005%) to 92.691% when pulling 118b049 on dcstone09:header_conflict into 30881e2 on node-nock:master.

@dcstone09
Copy link
Contributor Author

@ianwsperber Thanks, I've moved the test over.

@@ -196,6 +196,23 @@ test("reply should throw on error on the callback", function(t) {
req.end();
});

test('reply should not cause an error on header conflict', function (t) {
Copy link

Choose a reason for hiding this comment

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

@ianwsperber This looks good.

@@ -73,6 +73,7 @@ Interceptor.prototype.reply = function reply(statusCode, body, rawHeaders) {

if (this.scope._defaultReplyHeaders) {
headers = headers || {};
headers = common.headersFieldNamesToLowerCase(headers);
Copy link

Choose a reason for hiding this comment

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

@iangreenleaf This corrects the throwing conflict exception. Do we have test cases for testing overrides of the default headers when strings are loosely matching ('SomEthing' ==='something')?

Copy link
Contributor

Choose a reason for hiding this comment

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

@moaxaca Not sure off the top of my head

Copy link

Choose a reason for hiding this comment

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

@ianwsperber This test could over that.

Copy link

@felipead felipead 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

});
});

t.end()
Copy link
Contributor

Choose a reason for hiding this comment

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

@dcstone09 Before ending the test, could we actually make a request to http://www.website.com/search?IMO that would be a more complete assertion of the expected behavior.

@coveralls
Copy link

coveralls commented Apr 18, 2017

Coverage Status

Coverage increased (+0.005%) to 92.691% when pulling eb5be6a on dcstone09:header_conflict into f268187 on node-nock:master.

@dcstone09
Copy link
Contributor Author

@ianwsperber The test case has been updated

Copy link
Member

@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

@gr2m gr2m merged commit 8fd0942 into nock:master Feb 12, 2018
@lock
Copy link

lock bot commented Sep 13, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you!

@lock lock bot locked as resolved and limited conversation to collaborators Sep 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants