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

Fixing default statusCode (302) on response.redirect() #36

Merged

Conversation

estilles
Copy link

Fixing issue #25

Setting default statusCode to 302 on response.redirect('/url') (i.e. when statusCode is not specified in call).
Modified unit test to reflect this change.

@estilles
Copy link
Author

According to Express 4.x docs for res.redirect() ...

If you don’t specify status, the status code defaults to “302 “Found”.

I submitted this PR under the assumption that node-mocks-http is emulating Express. If that's not the case I will withdraw the PR.

@howardabrams
Copy link
Collaborator

The original goal was to work with both Node's standard API as well as Express', however, this does seem like a good idea.

However, this change breaks the tests. Do you have time to investigate? Otherwise, I'll try to see what's up later.

@estilles
Copy link
Author

estilles commented Mar 1, 2015

I do have time. I'll check it out and get back to you.

Thank you for this mock, BTW. It's a great project and I'm happy to contribute.

On a separate note, have you considered using a continuous integration service like Travis CI. It independently builds and runs the test suite on pull request (or sets of commits to master) and determines whether or not the buld is passing. In includes email notifications on build status and a badge you can put on your README to show the status. And, best of all, it's FREE for open source proyects. :-)

@estilles
Copy link
Author

estilles commented Mar 1, 2015

@howardabrams, not sure if you noticed but I included a in the PR a commit that modifies the "redirect - Redirect to a url without response code" test in test-mockResponse.js to account for the change in behavior.

I ran the tests locally and they pass (see screenshot below).
screen shot 2015-03-02 at 07 07 15
Are you getting a different result?

howardabrams added a commit that referenced this pull request Mar 5, 2015
…n-redirect

Fixing default statusCode (302) on response.redirect()
@howardabrams howardabrams merged commit e61617d into eugef:master Mar 5, 2015
@estilles estilles deleted the hotfix/fixing-statuscode-on-redirect branch March 5, 2015 05:01
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

2 participants