Skip to content
This repository has been archived by the owner on Jun 18, 2024. It is now read-only.

Fix HttpClientWebRequest so that it supports both GET and POST requests. #461

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

Conversation

dconnelly
Copy link

Should fix #452

@azurecla
Copy link

Hi @dconnelly, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.azure.com.

TTYL, AZPRBOT;

@codecov-io
Copy link

Current coverage is 10.68%

Merging #461 into master will increase coverage by +0.26% as of bbab906

@@            master    #461   diff @@
======================================
  Files          550     550       
  Stmts        20432   20440     +8
  Branches      2625    2628     +3
  Methods          0       0       
======================================
+ Hit           2131    2183    +52
- Partial        153     157     +4
+ Missed       18148   18100    -48

Review entire Coverage Diff as of bbab906

Powered by Codecov. Updated on successful CI builds.

@azurecla
Copy link

@dconnelly, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, AZPRBOT;

@serious6
Copy link
Member

@dconnelly thanks for your contribution. Maybe you can add tests that declares initialization of HttpClientWebRequest to be working with different request methods.

@dconnelly
Copy link
Author

Sure I can add some unit tests.

Also, are there plans for actual integration tests with real accounts? Perhaps Office365 with some test accounts would be a good start, and should improve code coverage numbers considerably.

@serious6
Copy link
Member

Yes that would be nice. AFAIK we might ran into troubles while credentials must not be exposed and Microsoft needs to donate some accounts.

@dconnelly
Copy link
Author

Isn't this a Microsoft project? ;-)

Probably all is needed is a few O365 test accounts and Travis does support storing encrypted items in .travis.yml (see http://docs.travis-ci.com/user/encryption-keys/).

@serious6
Copy link
Member

maybe @vboctor can check on the integration testing topic and the related accounts. Meanwhile some unit-tests could be added to have this merged ;-)

@serious6
Copy link
Member

serious6 commented Dec 2, 2015

@dconnelly are you planning any changes so far?

@dconnelly
Copy link
Author

Hey sorry André been swamped. Will get to it this weekend.

  • David

On Wed, Dec 2, 2015 at 1:41 PM, André Behrens [email protected]
wrote:

@dconnelly https://github.com/dconnelly are you planning any changes so
far?


Reply to this email directly or view it on GitHub
#461 (comment)
.

@serious6
Copy link
Member

@dconnelly it would be nice if we could merge your contribution. Please provide the mentioned changes if you are able to.

@kentongray
Copy link
Contributor

running into this issue too in my opinion this should just be merged. I verified the fix worked so we are running on master + this change on our production instance

@dconnelly dconnelly closed this Feb 15, 2016
@dconnelly dconnelly deleted the autodiscover_fix branch February 15, 2016 02:40
@dconnelly dconnelly restored the autodiscover_fix branch February 15, 2016 02:49
@dconnelly
Copy link
Author

André, I added some unit test coverage although it's difficult to test fully without an actual server connection. Let me know what you think.

@@ -0,0 +1,42 @@
package microsoft.exchange.webservices.data.core;
Copy link
Member

Choose a reason for hiding this comment

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

@dconnelly license header must be added.

@serious6
Copy link
Member

@dconnelly once license header has been added this will be merged. Thanks for your contribution.

@MarkRBM
Copy link

MarkRBM commented Dec 2, 2016

@serious6 can you tell me the line of text you want added and the file to add it and I will make a commit? madness that this has not been merged for over a year

@kentongray
Copy link
Contributor

still waiting on this one

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants