Purify-net / PUrify

PUrify
Apache License 2.0
11 stars 6 forks source link

HttpWebRequest/HttpClient and WebClient integration tests #21

Closed Mpdreamz closed 10 years ago

Mpdreamz commented 10 years ago

This PR adds several integration tests for HttpWebRequest/HttpClient and WebClient to make sure the purified uri we give these classes also results in a "purified" request to the server.

It does this by doing requests to http://request-path.herokuapp.com

The source code of which is here:

https://github.com/Mpdreamz/request-path

Interestingly deploying this on azure: http://request-path.azurewebsites.net/hello%2F-world/

vs heroku: http://request-path.herokuapp.com/hello%2F-world/

yields different results! %2F truly is the bane of the microsoft stack :)

PS: if you want access to the herokuapp/git repos let me know!

PS PS: this PR also moved the extension method from the Purify namespace to the PUrify namespace. I'm not sure if the lowercase u was intentional or not? If so I'll need to ammend this PR.

glennblock commented 10 years ago

@Mpdreamz why not self-host and use that for the test? I really don't want to have an acceptance test relying on an external server.

Mpdreamz commented 10 years ago

Hi @glennblock

Self hosting was indeed the first route I went with using owin:

using (var server = CreateServer( async env =>
{
    var urlOnServer = env.Get<string>("owin.RequestPath");
    var responseStream = env.Get<Stream>("owin.ResponseBody");

    using (var stream = new MemoryStream(Encoding.UTF8.GetBytes(urlOnServer)))
        await stream.CopyToAsync(responseStream, 1024);
}))
{
    var client = new HttpClient();
    var uri = new Uri("http://localhost:3002/hello-%2F/world");
    uri.Purify()
    var result = await client.PostAsync(uri, new StringContent("ads"));
    Assert.True(result.IsSuccessStatusCode);
    var body = await result.Content.ReadAsStringAsync();
    body.ShouldEqual("/hello-%2F/world");
}

This was using Nowin but it was reporting an incorrect "owin.RequestPath" itself which was rather discouraging,

That's why i went with a sure thing.

I'll try and rewrite the tests to use a self hosted webapi/mvc/nancyfx in the comming days hopefully they fair better.

I'm pleased to have seen all the different clients picking up purified uri's as expected though :)

glennblock commented 10 years ago

That's great to hear! I understand the Nowin issue, but I feel pretty strongly that we don't want to pull in a cloud dependency for unit tests.

On Thu, Jan 16, 2014 at 12:38 PM, Martijn Laarman notifications@github.comwrote:

Hi @glennblock https://github.com/glennblock

Self hosting was indeed the first route I went with using owin:

using (var server = CreateServer( async env => { var urlOnServer = env.Get("owin.RequestPath"); var responseStream = env.Get("owin.ResponseBody");

using (var stream = new MemoryStream(Encoding.UTF8.GetBytes(urlOnServer)))
    await stream.CopyToAsync(responseStream, 1024);

})) { var client = new HttpClient(); var uri = new Uri("http://localhost:3002/hello-%2F/world"); uri.Purify() var result = await client.PostAsync(uri, new StringContent("ads")); Assert.True(result.IsSuccessStatusCode); var body = await result.Content.ReadAsStringAsync(); body.ShouldEqual("/hello-%2F/world"); }

This was using Nowin but it was reporting an incorrect "owin.RequestPath" itself which was rather discouraging,

That's why i went with a sure thing.

I'll try and rewrite the tests to use a self hosted webapi/mvc/nancyfx in the comming days hopefully they fair better.

I'm pleased to have seen all the different clients picking up purified uri's as expected though :)

— Reply to this email directly or view it on GitHubhttps://github.com/glennblock/PUrify/pull/21#issuecomment-32544451 .

Mpdreamz commented 10 years ago

No worries I can totally get behind that conviction :+1: I'll get cracking on reworking it as soon as I can.

Just to nitpick though thats why I called them integration test vs unit tests :)

Mpdreamz commented 10 years ago

The namespace of the Purify() extension method is Purify whilst all the other code lives in PUrify. Is this intentional?

glennblock commented 10 years ago

Definitely not, just a typo.

On Thu, Jan 16, 2014 at 12:53 PM, Martijn Laarman notifications@github.comwrote:

The namespace of the Purify() extension method is Purify whilst all the other code lives in PUrify. Is this intentional?

— Reply to this email directly or view it on GitHubhttps://github.com/glennblock/PUrify/pull/21#issuecomment-32545732 .

glennblock commented 10 years ago

@Mpdreamz the nomenclature is not the problem. It's having a build that relies on an external resource (ie on Azure/Heroku) that will likely be down at times and maybe cause the build to fail giving false negatives. I don't think that is acceptable.

Mpdreamz commented 10 years ago

IntegrationTests by nature are always written against services out of your control (i.e databases, 3rd party webservices). I see them as end to end tests that may or may not be part of the build.

Don't mean to dwell (nag even?:)) on this though, semantic schmementics. I was lazy and just wanted to see it work lol, thanks for the pushback.

Getting them as part of the unit tests and the build is much more valuable indeed!

glennblock commented 10 years ago

I'll keep nitpicking, yes that's true and sometimes it is unavoidable, but in this case it's not the same. This is NOT a service, it's a generic web server, it's not like talking to S3 ;-). We ran lots of integration tests WITHIN Microsoft, but we didn't generally put them on the public facing CI servers i.e. Travis etc. Maybe it's a pet peeve, but I resort to these kinds of tests as a last resort and in this case I think we have a solution.

On Thu, Jan 16, 2014 at 1:20 PM, Martijn Laarman notifications@github.comwrote:

IntegrationTests by nature are always written against services out of your control (i.e databases, 3rd party webservices). I see them as end to end tests that may or may not be part of the build.

Don't mean to dwell (nag even?:)) on this though, semantic schmementics. I was lazy and just wanted to see it work lol, thanks for the pushback.

Getting them as part of the unit tests and the build is much more valuable indeed!

— Reply to this email directly or view it on GitHubhttps://github.com/glennblock/PUrify/pull/21#issuecomment-32548208 .

glennblock commented 10 years ago

@Mpdreamz ok, I am done being difficult now :-)

Mpdreamz commented 10 years ago

Right! I think we're on the same page :beer: Now lets hope a self hosted asp.net server behaves better or we'll have come full circle on this lol :)

Having seen this work (whoohoo) I'm all for getting a nuget package out the door asap? Whats your stance on this?