search

Purify-net / PUrify

PUrify
Apache License 2.0
11 stars 6 forks source link

Feature/httpclient tests #24

Closed Mpdreamz closed 10 years ago

Mpdreamz commented 10 years ago

Finally had a chance to sit down for this again.

HttpClient tests now use HttpListener self hosting, this exposes an HttpListenerContext to the Owin dictionary where we can get the proper non-urldecoded requested url from.

glennblock commented 10 years ago

Hi @Mpdreamz

Thanks for the effort here.

My main feedback would be on naming. The test names are not really descriptive of what the expected outcome is, and each tests performs multiple things.

I'd prefer you use the approach @haacked outline here: http://haacked.com/archive/2012/01/02/structuring-unit-tests.aspx/ which is what I've used in the first set of unit tests.

So I would have an outer class for each scenario and then have methods for each different test / observation

When a test fails we should clearly know exactly what that test is doing, this will also help people who are reading the test code.

glennblock commented 10 years ago

Also I would try if possible to stick to a single assert per actual XUnit test. It's not black and white, but a general rule. Using the outer class helps here because you can put some shared initialization code in the constructor of that class. We do that quite a bit in the scriptcs codebase.

Here's an example the relies on this idea of initializing common code in the ctor here: https://github.com/scriptcs/scriptcs/blob/dev/test/ScriptCs.Hosting.Tests/RuntimeServicesTests.cs#L17

Mpdreamz commented 10 years ago

Naming things, i still suck at it :) nice!

Sorry for not following the convention i've amended the PR.

glennblock commented 10 years ago

Getting much better, now I have just one more ask :-)

Move all the setup to constructors, save the value you need to test in a field/fields and make the test method itself just an assertion.

I helps because it's really easy to see what you are checking for. Because you now have a class you can take advantage of it by having scoped members.

Mpdreamz commented 10 years ago

Ahh but that means doing async "requests" from the constructors (which you cant await) because what i am testing is the response.. which doesnt't seem very pragmatic?

glennblock commented 10 years ago

You can just use task.Result. Don't use async. It works trust me.

On Thursday, February 6, 2014, Martijn Laarman notifications@github.com wrote:

Ahh but that means doing async "requests" from the constructors (which you cant await) because what i am testing is the response.. which doesnt't seem very pragmatic?

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

glennblock commented 10 years ago

If it is a huge deal don't worry about it

On Thursday, February 6, 2014, Martijn Laarman notifications@github.com wrote:

Naming things, i still suck at it :) nice!

Sorry for not following the convention i've amended the PR.

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

Mpdreamz commented 10 years ago

No big deal, I'm following your lead on this :+1:

Mpdreamz commented 10 years ago

amened once more, keep throwing big deals at me glenn :+1:

glennblock commented 10 years ago

Home run! Thanks!

glennblock commented 10 years ago

Thanks Dude!

On async, this hit me when I was building the integration tests for Web API book. Basically all the tests use an HttpClient wired directly to the server. It's all async though so I use Task.Result.

On Thu, Feb 6, 2014 at 2:37 AM, Martijn Laarman notifications@github.comwrote:

amened once more, keep throwing big deals at me glenn [image: :+1:]

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

glennblock commented 10 years ago

Here's an example: https://github.com/webapibook/issuetracker/blob/BuildingTheApi/test/WebApiBook.IssueTrackerApi.AcceptanceTests/Features/RetrievingIssues.cs#L25

This uses XBehave which I really like for acceptance style tests, but I didn't want to push you too hard :p

On Thu, Feb 6, 2014 at 2:37 AM, Martijn Laarman notifications@github.comwrote:

amened once more, keep throwing big deals at me glenn [image: :+1:]

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