Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngMock): respond did not always take a statusText argument #8270

Closed
wants to merge 1 commit into from
Closed

fix(ngMock): respond did not always take a statusText argument #8270

wants to merge 1 commit into from

Conversation

shahata
Copy link
Contributor

@shahata shahata commented Jul 19, 2014

minor fix so that respond can take a statusText argument even when having status 200 by default

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#8270)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@shahata shahata added cla: yes and removed cla: no labels Jul 19, 2014
@Narretz Narretz added this to the Backlog milestone Jul 21, 2014
minor fix so that `respond` can take a `statusText` argument even when having status 200 by default
@pkozlowski-opensource
Copy link
Member

LGTM. I would probably just add the test you've created without deleting the existing one. WDYT?

@pkozlowski-opensource pkozlowski-opensource modified the milestones: 1.3.x, Backlog, 1.3.5 Nov 23, 2014
@pkozlowski-opensource pkozlowski-opensource self-assigned this Nov 23, 2014
@shahata
Copy link
Contributor Author

shahata commented Nov 25, 2014

@pkozlowski-opensource I think it's redundant, it tests the exact same thing, just with more lines of code and unneeded assertions...

@googlebot
Copy link

CLAs look good, thanks!

@pkozlowski-opensource
Copy link
Member

@shahata hmm, I still would feel a bit safer having those 2 tests as those are not exactly the same: the one you've deleted don't have status text, do 2 requests. Up to you but since tests are cheep I would prefer to be safe than sorry.

@shahata shahata closed this in 08cd5c1 Nov 26, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants