-
Notifications
You must be signed in to change notification settings - Fork 69
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
Split out Should and ShouldTests #124
base: master
Are you sure you want to change the base?
Conversation
Added Shouldly and ShouldlyTests Added editorconfig Updated references
@@ -17,12 +17,12 @@ public class NestedMagicProvider : Behavior<ILikeMagic> | |||
{ | |||
public override void Given(ILikeMagic instance) | |||
{ | |||
instance.CalledByDuringGiven.Add(this.GetType().Name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
R# pointed this out. I was just looking through warnings and messages and saw this.
@@ -7,7 +7,8 @@ namespace SpecsFor.Demo.PlainBDD | |||
{ | |||
public class OrderProcessorSpecs | |||
{ | |||
public class given_the_item_is_available_when_processing_an_order : SpecsFor<OrderProcessor> | |||
[System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE1006:Naming Styles", Justification = "BDD naming style")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming style here was clogging up the warnings window, so I added the attributes to shut them up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The underscore style of naming specs never really caught on outside a few BDD frameworks. I'm cool with renaming the specs the specs to match standard conventions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Not saying that should hold up merging this PR, just noting that for going forward)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually a big fan of the underscores because they make it possible to differentiate_between_regular_words_and_CompundClassNames
I'm not saying to change them at all. I just like to acknowledge warnings so that they don't block my view of other things that might be more important.
@@ -1,9 +1,9 @@ | |||
using NUnit.Framework; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are mostly new files in a new location, but git has noticed the similarity to the old tests, so that might be a little confusing. The tests have been removed from AutoFac.Tests and StructureMap.Tests, and added to a new "Should.Tests" project.
@@ -1,6 +1,6 @@ | |||
using System; | |||
|
|||
namespace SpecsFor.Core.ShouldExtensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're not really "extensions" in the traditional sense, so I simplified the new namespace.
@@ -1,9 +1,9 @@ | |||
using NUnit.Framework; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This starts the new second copy of the extensions and tests for the Shouldly library.
}); | ||
}); | ||
} | ||
[Test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These vanished on me. I put them back, but Git is obsessing over the whitespace for some reason.
Are you a "tabs" guy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not anymore. I was in the "tabs" camp, but tabs have definitely lost, and I'm ok with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with breaking the hard-dependency on Should, but I think we should shoot for a single "assertions" helper package. Maybe our packages could look like:
SpecsFor.StructureMap
SpecsFor.AutoFac
SpecsFor.Assertions (not 100% in love with this name)
I've got some EF-specific stuff that I reuse on almost every project, too, that I'd like to package up into something reusable, so maybe eventually we could have:
SpecsFor.EntityFramework
Or
SpecsFor.Assertions.EntityFramework
Thoughts?
I'm game to help make changes to consolidate things in this PR, too.
@@ -7,7 +7,8 @@ namespace SpecsFor.Demo.PlainBDD | |||
{ | |||
public class OrderProcessorSpecs | |||
{ | |||
public class given_the_item_is_available_when_processing_an_order : SpecsFor<OrderProcessor> | |||
[System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE1006:Naming Styles", Justification = "BDD naming style")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The underscore style of naming specs never really caught on outside a few BDD frameworks. I'm cool with renaming the specs the specs to match standard conventions.
@@ -7,7 +7,8 @@ namespace SpecsFor.Demo.PlainBDD | |||
{ | |||
public class OrderProcessorSpecs | |||
{ | |||
public class given_the_item_is_available_when_processing_an_order : SpecsFor<OrderProcessor> | |||
[System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE1006:Naming Styles", Justification = "BDD naming style")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Not saying that should hold up merging this PR, just noting that for going forward)
}); | ||
}); | ||
} | ||
[Test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not anymore. I was in the "tabs" camp, but tabs have definitely lost, and I'm ok with that.
|
||
namespace SpecsFor.Shouldly | ||
{ | ||
public static class Any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than duplicate the various helpers, I'd rather just have a single library that's agnostic to Should vs. Shouldy. Maybe we can ship a single "SpecsFor.AssertionHelpers" or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well certainly if the extension points for Should and Shouldly can both implement some kind of common interface, just enough to hang the extensions from, then the extensions could operate over that interface. This is just me spitballing from the couch though. I'd have to dig in and look at the structure to make a real recommendation.
This was just a first attempt at checking off a personal backlog item through refactoring. We've used Shouldly over Should for a while now. It's more frequently maintained, and provides nicer error messages. I split out the Should extensions from the Autofac and Structuremap assemblies and made them into their own independent SpecsFor.Should, and then added similar items for Shouldly. I'm sure some of this could be refactored back into Core, since large parts of it don't actually reference either library, but I thought this was a good first "thing" to work on.
I added a pretty standard editorconfig file to lock down coding styles for the project. It's just lifted from other current projects of mine, and may not match your styles 100%, but it's a start.
I updated package references that were at least "in range". The demo project is still referencing SpecsFor v6.0.0, but it's the full release now instead of the alpha. I'd update it to 7, but that requires some rewriting. Autofac.Extras.Moq has jumped from 4.2 to 6.0, so I left that one alone for now as well. Two whole point releases seems dangerous to me. Maybe I'll swing back around to this later on. Everything else was at most a minor release or two, and seems to have updated without any issue. That's no guarantee of compatibility, but all the tests pass.
I have no clue about building NuGet packages, but I took a stab at it based on what was already there. It's probably all wrong, but I'd like to learn.