Add netcoreapp1.0 test target to Nancy.Tests #2634
Conversation
|
Hi @blairconrad, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
src/Nancy/Json/Simple/SimpleJson.cs
Outdated
| return type.GetProperty(name); | ||
| #endif | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm not sure if this file is supposed to be modified or not…
There was a problem hiding this comment.
Ideally as little as possible
| @@ -44,9 +44,13 @@ public IEnumerable<InstanceRegistration> InstanceRegistrations | |||
|
|
|||
| private static void ThrowWhenNoAppStartupsFixtureRuns() | |||
| { | |||
| #if CORE | |||
There was a problem hiding this comment.
I used CORE since I saw it in Nancy/project.json. Happy to change to something else. Or something elses.
There was a problem hiding this comment.
To be honest, since this is in a test fixture.. we could probably ditch the #ifdef and just use this approach for both targets?
There was a problem hiding this comment.
I'd thought the same. Thanks.
| @@ -44,9 +44,13 @@ public IEnumerable<InstanceRegistration> InstanceRegistrations | |||
|
|
|||
| private static void ThrowWhenNoAppStartupsFixtureRuns() | |||
| { | |||
| #if CORE | |||
| if ( Environment.StackTrace.Contains("Nancy.Tests.NoAppStartupsFixture")) | |||
There was a problem hiding this comment.
less robust than StackTrace().GetFrames().
Although if you're not worried about it, it might be cleaner to collapse to this.
There was a problem hiding this comment.
Could we use typeof(NoAppStartupsFixture).Name instead?
There was a problem hiding this comment.
Of course. Silly of me.
| @@ -10,8 +10,8 @@ | |||
|
|
|||
| namespace Nancy.Tests.Resources { | |||
| using System; | |||
|
|
|||
|
|
|||
| using Nancy.Extensions; | |||
There was a problem hiding this comment.
I am absolutely sure that editing the Designer files was a bad idea, but I didn't have any good ones. I'm all ears.
| @@ -70,6 +70,7 @@ public void Request_should_be_available_to_request_startup() | |||
| this.bootstrapper.RequestStartupLastRequest.ShouldBeSameAs(request); | |||
| } | |||
|
|
|||
| #if !CORE | |||
There was a problem hiding this comment.
This was mostly laziness/expediency. I just haven't found the dynamic generator for the new framework yet.
| #endif | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
nicked from FakeItEasy
There was a problem hiding this comment.
can we add XML comments on this? class, ctor and methods
|
@blairconrad, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
| @@ -1,6 +1,8 @@ | |||
| { | |||
| "version": "1.4.1.0", | |||
There was a problem hiding this comment.
Without this, CookieBasedSessionsFixture.Should_load_valid_test_data fails because the DefaultObjectSerializer can't find Nancy.Tests, Version=1.4.1.0, Culture=neutral, PublicKeyToken=null, which was presumably encoded inside CookieBasedSessionsFixture.ValidData.
The full stack trace of the failure is
System.IO.FileLoadException : Could not load file or assembly 'Nancy.Tests, Version=1.4.1.0, Culture=neutral, PublicKeyToken=null'. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)
Stack Trace:
at System.RuntimeTypeHandle.GetTypeByName(String name, Boolean throwOnError, Boolean ignoreCase, Boolean reflectionOnly, StackCrawlMarkHandle stackMark, IntPtr pPrivHostBinder, Boolean loadTypeFromPartialName, ObjectHandleOnStack type, ObjectHandleOnStack keepalive)
at System.RuntimeTypeHandle.GetTypeByName(String name, Boolean throwOnError, Boolean ignoreCase, Boolean reflectionOnly, StackCrawlMark& stackMark, IntPtr pPrivHostBinder, Boolean loadTypeFromPartialName)
at System.RuntimeType.GetType(String typeName, Boolean throwOnError, Boolean ignoreCase, Boolean reflectionOnly, StackCrawlMark& stackMark)
at System.Type.GetType(String typeName)
at CallSite.Target(Closure , CallSite , Type , Object )
src\Nancy\DefaultObjectSerializer.cs(69,0): at Nancy.DefaultObjectSerializer.Deserialize(String sourceString)
src\Nancy\Session\CookieBasedSessions.cs(190,0): at Nancy.Session.CookieBasedSessions.Load(Request request)
test\nancy.tests\Unit\Sessions\CookieBasedSessionsFixture.cs(370,0): at Nancy.Tests.Unit.Sessions.CookieBasedSessionsFixture.Should_return_blank_session_if_hmac_changed()
System.IO.FileLoadException : Could not load file or assembly 'Nancy.Tests, Version=1.4.1.0, Culture=neutral, PublicKeyToken=null'. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)
Stack Trace:
at System.RuntimeTypeHandle.GetTypeByName(String name, Boolean throwOnError, Boolean ignoreCase, Boolean reflectionOnly, StackCrawlMarkHandle stackMark, IntPtr pPrivHostBinder, Boolean loadTypeFromPartialName, ObjectHandleOnStack type, ObjectHandleOnStack keepalive)
at System.RuntimeTypeHandle.GetTypeByName(String name, Boolean throwOnError, Boolean ignoreCase, Boolean reflectionOnly, StackCrawlMark& stackMark, IntPtr pPrivHostBinder, Boolean loadTypeFromPartialName)
at System.RuntimeType.GetType(String typeName, Boolean throwOnError, Boolean ignoreCase, Boolean reflectionOnly, StackCrawlMark& stackMark)
at System.Type.GetType(String typeName)
at CallSite.Target(Closure , CallSite , Type , Object )
src\Nancy\DefaultObjectSerializer.cs(69,0): at Nancy.DefaultObjectSerializer.Deserialize(String sourceString)
src\Nancy\Session\CookieBasedSessions.cs(190,0): at Nancy.Session.CookieBasedSessions.Load(Request request)
test\nancy.tests\Unit\Sessions\CookieBasedSessionsFixture.cs(355,0): at Nancy.Tests.Unit.Sessions.CookieBasedSessionsFixture.Should_load_valid_test_data()
This makes me a little concerned about the behaviour of DefaultObjectSerializer, especially if it's used to serialize and deserialize long-lived data, but I think that's outside the scope of this PR.
There was a problem hiding this comment.
too tired to make sense of this.. remind me to look at this again :D
There was a problem hiding this comment.
Don't forget to look at this again. Instead of changing the version of this assembly, we could change the test data to the values @thomaslevesque suggested in #2634 (comment).
Of course, either change would only fix the test. There's a larger issue of whether DefaultObjectSerializer should able to deserialize types into editions of those types from different-versioned assemblies, but that's beyond this PR. I could spin off an issue, if you like.
There was a problem hiding this comment.
Yeah this needs to be addressed separately. We cannot have one of our tests depend on a specific version. @jchannon
There was a problem hiding this comment.
I'm not entirely sure where to go from this comment. Are you suggesting to leave the code as is for now and then someone (you, me, @jchannon, a roving Samaritan) will "fix" the test separately?
There was a problem hiding this comment.
I'm suggesting that @jchannon takes a look since he implemented DefaultObjectSerializer. To be honest we changed the implementation, to it's current form, when we moved to core. The old implementation dependency on the binary formatter which was removed from .net core. Maybe it would even be wise to see if the binary formatter is back and make a call if we should revert back or not?
There was a problem hiding this comment.
Understood. The removal was a small pain point for us over at FakeItEasy too.
I thought I'd heard that binary serialization was coming back sometime, but I cannot find a reference now.
|
With my latest change, |
|
Oh, I should've said. @thomaslevesque was a big help in debugging the |
|
FWIW, the correct values for
Me too. If I understand correctly, it means that deploying a new version of an app will always invalidate session cookies. |
|
Well, that's the AppVeyor build sorted. |
|
Huzzah! Green builds. The netcoreapp1.0 Nancy.Tests aren't run on Travis (yet), but I'll look at that later. Then we can wrangle over details. |
blairconrad
changed the title
|
Okay. We've a functional PR. Nancy.Tests has netcoreapp1.0 tests that run from the standard build script on both Windows and Unix (well, ubuntu at least). |
build.cake
Outdated
| - GetFiles("test/**/Nancy.ViewEngines.DotLiquid.Tests.xproj") | ||
| - GetFiles("test/**/Nancy.ViewEngines.Markdown.Tests.xproj") | ||
| - GetFiles("test/**/Nancy.ViewEngines.Razor.Tests.xproj") | ||
| - GetFiles("test/**/Nancy.ViewEngines.Razor.Tests.Models.xproj"); |
There was a problem hiding this comment.
The long list is a little ugly (and repeated), but I opted to blacklist the projects that hadn't been modernized yet so any new projects that are added will be caught.
|
Hey, @khellang, @thecodejunkie, and @jchannon. I'm bothering you specifically because I've seen you committing recently, so I know you're active. |
|
@blairconrad sorry.. this is on my radar but I've been working on getting the |
|
@thecodejunkie, no worries. I understand about competing priorities. I'm in no rush, and wouldn't've said anything except I couldn't even tell if the PR had been noticed. Happy (belated) Christmas. I'll be celebrating on the 25th, so out of communication for a day or two. I noticed the update to the build scripts. While resolving conflicts, I messed up your line endings, so just now repushed the last two commits. TL;DR:
|
src/Nancy/Json/Simple/SimpleJson.cs
Outdated
| @@ -1787,6 +1787,15 @@ public static IEnumerable<PropertyInfo> GetProperties(Type type) | |||
| #endif | |||
| } | |||
|
|
|||
| public static PropertyInfo GetProperty(Type type, string name) | |||
There was a problem hiding this comment.
This is more appropriate to live as an extension method in TypeExtensions and also add XML documentation
There was a problem hiding this comment.
As it turns out, throwing a .GetTypeInfo() on the call site removed the need for this. Reverting.
src/Nancy/Json/Simple/SimpleJson.cs
Outdated
| return type.GetProperty(name); | ||
| #endif | ||
| } | ||
|
|
There was a problem hiding this comment.
Ideally as little as possible
| @@ -44,9 +44,13 @@ public IEnumerable<InstanceRegistration> InstanceRegistrations | |||
|
|
|||
| private static void ThrowWhenNoAppStartupsFixtureRuns() | |||
| { | |||
| #if CORE | |||
| if ( Environment.StackTrace.Contains("Nancy.Tests.NoAppStartupsFixture")) | |||
There was a problem hiding this comment.
Could we use typeof(NoAppStartupsFixture).Name instead?
| @@ -44,9 +44,13 @@ public IEnumerable<InstanceRegistration> InstanceRegistrations | |||
|
|
|||
| private static void ThrowWhenNoAppStartupsFixtureRuns() | |||
| { | |||
| #if CORE | |||
There was a problem hiding this comment.
To be honest, since this is in a test fixture.. we could probably ditch the #ifdef and just use this approach for both targets?
| @@ -327,7 +327,7 @@ protected override INancyEngine GetEngineInternal() | |||
| { | |||
| if (this.ShouldThrowWhenGettingEngine) | |||
| { | |||
| throw new ApplicationException("Something when wrong when trying to compose the engine."); | |||
| throw new Exception("Something when wrong when trying to compose the engine."); | |||
There was a problem hiding this comment.
Why change this exception type?
There was a problem hiding this comment.
No ApplicationException in .NET Core. I'm happy to use another type, if you like.
There was a problem hiding this comment.
Na, fair enough. Let's use Exception since it's only in the context of the test itself
| @@ -37,25 +37,17 @@ public void Should_stringify_an_expiry_to_gmt_and_stupid_format() | |||
| } | |||
|
|
|||
| [Fact] | |||
| [UsingCulture("fr-FR")] | |||
There was a problem hiding this comment.
test name says "to english" but the used culture is french? =)
There was a problem hiding this comment.
Yes. It's an allusion to my Canadian heritage.
No, but seriously, that's how I found it. And it does render the expiry in English.
If you'd like me to rename while I'm here, I can, but like the "de-DE" change, I think it's technically outside the scope of this PR.
Leaving as is, unless you say more.
|
|
||
| using FakeItEasy; | ||
|
|
||
| using Nancy.Bootstrapper; | ||
| using Nancy.Cryptography; | ||
| using Nancy.Helpers; |
There was a problem hiding this comment.
The changes in this file does not seem to be needed - revert?
There was a problem hiding this comment.
We had no System.Web in .NET Core. Nancy.Helpers had all the missing types.
If you think it's inappropriate to use those here, I can pursue another approach.
There was a problem hiding this comment.
Are you saying that tests, in this fixture, uses extension methods that came from System.Web, but since it's not on Core you added in Nancy.Helpers since it had matching extensions that could be used instead?
There was a problem hiding this comment.
Nearly. It's an entire missing class. System.Web.HttpUtility. My understanding is that System.Web is not available in .NET Core.
Nancy.Helpers.HttpUtility is used in the production code, e.g. in NancyCookie, so I took advantage here. Are you concerned that it weakens the tests?
| @@ -26,7 +27,8 @@ public class StaticContentConventionBuilderFixture | |||
|
|
|||
| public StaticContentConventionBuilderFixture() | |||
| { | |||
| this.directory = Environment.CurrentDirectory; | |||
| this.directory = new DirectoryInfo(typeof(StaticContentConventionBuilderFixture).GetAssembly().Location) | |||
There was a problem hiding this comment.
Environment.CurrentDirectory does not exist in .NET Core.
We can achieve the same effect by using System.IO.Directory.GetCurrentDirectory().
For some reason I thought this didn't work, though. In FakeItEasyLand it definitely didn't, but the external files Nancy needs may be better distributed. I'll update to GetCurrentDirectory. Unless the linux build hates it (and I'm way too lazy to try that until I've worked through everything else in the review).
| @@ -1,6 +1,8 @@ | |||
| { | |||
| "version": "1.4.1.0", | |||
There was a problem hiding this comment.
too tired to make sense of this.. remind me to look at this again :D
| #endif | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
can we add XML comments on this? class, ctor and methods
|
Crap.. I wrote my review comments a week ago and forgot to submit them =/ No wonder they all said |
Ha! If I had a nickel for every time I've done that, I'd have $0.15! |
There was a problem hiding this comment.
Thanks for the comments, @thecodejunkie.
Bedtime for me now, so I'm pushing what changes I've made. I'll continue through your comments over the coming days—if I haven't said anything, I just haven't gotten to a comment yet; I'm not ignoring it. I'll address everything or explain why I think things should stay as they are.
| @@ -44,9 +44,13 @@ public IEnumerable<InstanceRegistration> InstanceRegistrations | |||
|
|
|||
| private static void ThrowWhenNoAppStartupsFixtureRuns() | |||
| { | |||
| #if CORE | |||
| if ( Environment.StackTrace.Contains("Nancy.Tests.NoAppStartupsFixture")) | |||
There was a problem hiding this comment.
Of course. Silly of me.
| @@ -44,9 +44,13 @@ public IEnumerable<InstanceRegistration> InstanceRegistrations | |||
|
|
|||
| private static void ThrowWhenNoAppStartupsFixtureRuns() | |||
| { | |||
| #if CORE | |||
There was a problem hiding this comment.
I'd thought the same. Thanks.
| @@ -327,7 +327,7 @@ protected override INancyEngine GetEngineInternal() | |||
| { | |||
| if (this.ShouldThrowWhenGettingEngine) | |||
| { | |||
| throw new ApplicationException("Something when wrong when trying to compose the engine."); | |||
| throw new Exception("Something when wrong when trying to compose the engine."); | |||
There was a problem hiding this comment.
No ApplicationException in .NET Core. I'm happy to use another type, if you like.
| @@ -245,7 +245,7 @@ protected override INancyEngine GetEngineInternal() | |||
| { | |||
| if (this.ShouldThrowWhenGettingEngine) | |||
| { | |||
| throw new ApplicationException("Something when wrong when trying to compose the engine."); | |||
| throw new Exception("Something when wrong when trying to compose the engine."); | |||
There was a problem hiding this comment.
No ApplicationException in .NET Core. I'm happy to use another type, if you like.
| @@ -3,6 +3,7 @@ | |||
| using Nancy.Diagnostics; | |||
| using System.Collections.Generic; | |||
| using System.Linq; | |||
| using System.Reflection; | |||
There was a problem hiding this comment.
GetMethods on line 73 needs it.
| @@ -436,6 +436,7 @@ public void Should_still_be_in_memory_when_more_bytes_have_been_written_to_strea | |||
| request.IsInMemory.ShouldBeTrue(); | |||
| } | |||
|
|
|||
| #if !CORE | |||
There was a problem hiding this comment.
Absolutely. Changing to
#if !CORE // BeginRead, EndRead, BeginWrite, EndWrite don't exist in .NET Core
|
I was surprised by the AppVeyor build failure, @thecodejunkie. |
|
@blairconrad odd, that file has been there for a while https://github.com/NancyFx/Nancy/commits/master/test/Nancy.Tests/Unit/AppDomainAssemblyCatalogFixture.cs .. I think the best thing to do is to continue going though the review comments, ignore that AppVeyor is failing (for now) and once all the fixes have been applied you rebase against latest master (it's needed anyway). If it is still a problem after that, then we'll look at it again |
|
I think the failing |
|
It's not a problem on |
|
@thecodejunkie, at the risk of being fractious, I can't agree. Have you tried it as I suggested? I see the failure on master on my personal machine. And I just now checked out master on my work computer (don't tell work) and got the same effect. Occasionally, not often, The very next run: Then 18 successes followed by then one pass and I think you've been lucky not to see the failure on the CI server. Or unlucky. |
|
As you may have noticed, I've been running some experiments with the build in a place you can see. Is this one known? |
|
Not a known one no :(
…On 8 February 2017 at 11:33, Blair Conrad ***@***.***> wrote:
As you may have noticed, I've been running some experiments with the build
in a place you can see.
After a few rebuilds of master, the Nancy build did fail
<https://ci.appveyor.com/project/NancyFx/nancy/build/2.0.0-ci0001840-unahedbd#L1397>,
but on a different test:
Nancy.Testing.Tests.BrowserFixture.Should_return_
JSON_serialized_querystring.
Is this one known?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2634 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGapuivuDdlZaeeldDU8Yk66PXlIwrxks5raagKgaJpZM4K7CYa>
.
|
|
Thanks for the speedy confirmation, @jchannon. I'm not trying to be difficult (it comes naturally), just trying to point out that maybe the tests have secrets. |
|
Nah go nuts! We REALLY appreciate your work here
…On 8 February 2017 at 11:39, Blair Conrad ***@***.***> wrote:
Thanks for the speedy confirmation, @jchannon
<https://github.com/jchannon>. I'm not *trying* to be difficult (it comes
naturally), just trying to point out that maybe the tests have secrets.
Any objection to me continuing to try to reproduce on AppVeyor the failure
that I see locally? It hogs the build a little, but I've never not seen it
start right away, and I will settle down once I go to work. 😃
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2634 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGapq0EvkpNL0XEmEK2l_qP1c55miVQks5raaltgaJpZM4K7CYa>
.
|
|
I think I've seen Update: Did b05ffc7 ever make it in? It's from #2098 (comment) which was closed without being merged. |
|
Thanks for chiming in, @xt0rted. I even see the failures from the command-line build. Either using |
|
try disabling parallel just in case
…On 8 February 2017 at 14:57, Blair Conrad ***@***.***> wrote:
Thanks for chiming in, @xt0rted <https://github.com/xt0rted>. I even see
the failures from the command-line build. Either using build.ps1 or
shortcutting and running a dotnet test -c Release on the relevant project.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2634 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGapnKnRYPkFyejaFy4PppMsDGc_szZks5radftgaJpZM4K7CYa>
.
|
|
It looks like the failing |
|
Oh, and we could potentially switch to FIE 3.0.0-rc001. |
|
sounds great =) |
|
Okay. There was a short delay while I made FIE 3.0.0-rc001, but I'll clean up the commits later today. |
- update FakeItEasy to 3.0.0-rc001 - set Nancy.Testing version to 1.4.1.0 to workaround failing deserialization
|
3.0.0-rc001ed! Rebased! Squashed! |
|
#2435 (comment) is where I had seen one of these tests failing. It's old but |
|
@blairconrad This is EPIC! Thanks a million ❤️ 👍 |
| @@ -255,6 +305,33 @@ Task("Test") | |||
| - GetFiles("./test/**/Nancy.Encryption.MachineKey.Tests.xproj") | |||
| - GetFiles("./test/**/Nancy.ViewEngines.DotLiquid.Tests.xproj") | |||
| - GetFiles("./test/**/Nancy.Embedded.Tests.xproj"); //Embedded somehow doesnt get executed on Travis but nothing explicit sets it | |||
|
|
|||
| var testProjects = GetFiles("./test/**/*.xproj"); | |||
There was a problem hiding this comment.
Does this mean on unix dotnet test is called and then mono is called as well?
There was a problem hiding this comment.
Yeah. I figured that would be desirable. Can be changed.
|
Hey, thanks for the merge, @jchannon. And to @thecodejunkie as well for the insightful comments during the review. |
|
🎉❤️ |
|
Embarrassingly, there's a warning generated when picking up the new FIE. I put the dependency as 3.0.0-rc001, not 3.0.0-rc001-build000097. I'm considering converting another test project, if nobody objects. I can include the fix there, or as a separate PR. |
specifically #2705 |
|
Awesome work @blairconrad ❤️ Thanks |
|
Glad to be of service, @thecodejunkie. |
Prerequisites
Description
Supports #2612.
A preliminary attempt to convert Nancy.Tests to target a netcoreapp1.0.
I expect I've done many things wrong, and invite corrections.
I'll highlight what I think are the most egregious violations.
update FakeItEasy to 3.0.0-alpha001
add workaround so FakeItEasy gets required Castle.Core-beta002
poorly patch the build so the project compiles and tests run
currently
CookieBasedSessionsFixture.Should_load_valid_test_datafailsDefaultNancyBootstrapperFixture.Container_should_ignore_specified_assembliesis conditionally compiled out