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

Nancy.Testing lovin' #633

Closed
thecodejunkie opened this Issue Jun 8, 2012 · 11 comments

Comments

Projects
None yet
5 participants
@thecodejunkie
Copy link
Member

thecodejunkie commented Jun 8, 2012

Nancy.Testing needs some love to polish out some of the rouge edges.

Currently we have open issues, assigned to 0.12, for the following things (this list is altered each time one of the items below are picked up and put into a separate issue)

  • #523 ConfigurableBootstrapper should prioritize rootpath provider from host
  • #495 Intercept the view model and make it available on the BrowserResponse
  • #421 Nancy.Testing should automatically load all assemblies it can find
  • #640 Replace HtmlAgilityPack with CsQuery
  • #643 Disable Auto Registration in ConfigurableBootstrapper
  • #647 Exclude Nancy.Testing from auto-registration
  • #645 Update the Nancy.Testing.Fakes.FakeNancyModule
  • #651 Remove xunit dependency from Nancy.Testing
  • #654 Improve dependency registration in ConfigurableBootstrapper

In addition here are some stuff I want to be looked at

  • View discovery Making sure Nancy.Testing finds views in the main project. Now you have to make sure the views are copied to the output folder (if this is the approach we want to take, then it needs to be documented!) or set up a root path provider that points to your main project. It can be a bit tricky to get it right on both windows/mono so we could possibly create a base class where you just told it which assembly (or a type and look up the assembly form that) you want to set the root path for
  • FakeModule Is being discovered by some bootstrappers and causes it to fail because the most strict constructor takes a string as a parameter. Not a good idea and the FakeModule API should (probably?) be rewritten to use a nested closure for configuration just like the Browser and ConfigurableBootstrapper. This should also be excluded from auto-discovery by bootstrappers Edit: this is done in #645
  • FakeRequest - Also make it use a nested closure for configuration. This should also be excluded from auto-discovery by bootstrappers
  • DisableAutoRegistration Auto registration should probably be turned of, by default, in the ConfigurableBootstrapper. Today you have to opt-out of it and since it has performance implications in large projects (and if you don't reuse your bootstrapper instance across tests). Changing into something like EnableAutoRegistration might be the direction to take Edit: this is done in #643
  • XUnit dependency Right now Nancy.Testing has a dependency on XUnit because the ShouldAssertions.cs is shipped in this file. Nancy.Testing should be agnostic to the framework that is being used and not force an xunit dependency on you. This means we will need to move / remove the ShouldAssertions elsewhere, possibly Nancy.Tests and update all other *.Tests projects to make sure they build. Also need to make sure that this doesn't mean the Rake file will run some tests multiple times (ie it should only run tests in distinct assembly names) Edit: this is done in #651
  • Other bootstrappers Make a sanity check and use the other bootstrappers with testing. Have gotten a few reports about modules not being loaded automatically. Figure out why and fix it ;)
  • CsQuery Investigate the possibility of replacing the usage of HtmlAgilityPack with CsQuery (https://github.com/jamietre/CsQuery) now that James created a nuget for it. Edit: this is done in #640
  • Dependency/Dependencies - Enable it so that you can register a single dependency, or collection of dependencies, by type using the ConfigurableBootstrapper. Currenctly you can only register instances Edit: this is done in #654

Each of these issues should be split out to a proper issue if/when you start working on it

If you are experiencing other concerns with Nancy.Testing, please add a comment in this issue!

@provegard

This comment has been minimized.

Copy link
Contributor

provegard commented Aug 2, 2012

I want to add something that I found confusing. I wanted to test that the content type of a response was application/json, so I wrote a test with the following assert:

Assert.AreEqual("application/json", result.Headers["Content-Type"]);

(Here, result is of type BrowserResponse.) This didn't work though, as the Headers dictionary was empty. I had to change the assert to:

Assert.AreEqual("application/json", result.Context.Response.ContentType);

I realize that I could have an integration test suite where I start a full-fledged server and send HTTP requests to it, but being able to test the headers as I first tried to do would bring my unit tests a little bit "closer to the browser."

Any thoughts on this?

@grumpydev

This comment has been minimized.

Copy link
Member

grumpydev commented Aug 2, 2012

@provegard - good idea, we should probably create this header (any others?) as a normal hosting provider would do.

@provegard

This comment has been minimized.

Copy link
Contributor

provegard commented Aug 3, 2012

Ah... Checking the Nancy code, I see now that Nancy doesn't translate anything into headers on its own, so doing it for a unit testing scenario may be a bit too "artificial." Especially if only a few select headers are translated.

Still, it was confusing not to see Content-Type in the dictionary. I do like my first test attempt better than the second.

I'm in two minds about this. :-)

@jrsconfitto

This comment has been minimized.

Copy link
Contributor

jrsconfitto commented Aug 13, 2012

i think i may have hit a dependency mismatch again with the Testing stuff. i'll look into it again to confirm, but after updating my xunit to the latest, i would get run time errors saying i wasn't using the 1.9.0.1566 version of xunit, even though the nuspec says anything greater than 1.7.0.1540 should work.

Fortunately, setting my NuGet packages to the 1.9.0.1566 version allowed everything to build and run all hunky-dory.

i can put together another PR on the subject, but i haven't looked to see if there are any current changes that would make this whole thing moot.

If i don't hear about anything i'll probably make another PR in a few days on the subject.

@grumpydev

This comment has been minimized.

Copy link
Member

grumpydev commented Aug 13, 2012

@jugglingnutcase the xunit dependency has been removed in the latest master - see #651

@codeprogression

This comment has been minimized.

Copy link
Contributor

codeprogression commented Aug 13, 2012

@thecodejunkie For the ConfigurableBootstrapper(Configurator), it would be nice to be able to hook into the ApplicationStartup and RequestStartup methods. Right now, I usually derive from ConfigurableBootstrapper to hook into ApplicationStartup. That way I can add things like an alternate FormsAuthentication.Enable(...) to the bootstrapper.

@jrsconfitto

This comment has been minimized.

Copy link
Contributor

jrsconfitto commented Aug 13, 2012

@grumpydev great, thanks for the heads up.

@thecodejunkie

This comment has been minimized.

Copy link
Member Author

thecodejunkie commented Aug 13, 2012

@codeprogression sounds useful, did you have a specific syntax in mind?

@codeprogression

This comment has been minimized.

Copy link
Contributor

codeprogression commented Aug 15, 2012

Hmm...how about:

c.ApplicationStartup((container,pipelines)=>{...});
c.RequestStartup((container,pipelines,context)=>{...});

I would think this would just add to the end of the App/Request startup methods. I can work something up and send a PR. I have the code in my head already. Just wonder how I would test it. Any particular test fixture that I can model after for ConfigurableBootstrapper?

@thecodejunkie

This comment has been minimized.

Copy link
Member Author

thecodejunkie commented Aug 22, 2012

Remaining issues will be sorted in 0.14

@grumpydev

This comment has been minimized.

Copy link
Member

grumpydev commented Jan 3, 2013

Last few are separate issues so closing this one down

@grumpydev grumpydev closed this Jan 3, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.