Skip to content

Conversation

mwhelan
Copy link
Member

@mwhelan mwhelan commented Jul 22, 2013

Hey @kkozmic

@JakeGinnivan here, I have been pairing today @mwhelan on making ConventionTests more extensible and getting rid of the base classes (pulled out into ConventionTests.Framework).

https://github.com/mwhelan/ConventionTests/blob/686186517dff35c2a723b0e7bb40788c0e792fc7/ConventionTests.Tests/DefaultConventionTests.cs is an example of how you would use convention tests to verify your conventions.

There are a few upsides from this change

  • No reliance on any unit test framework, it makes ConventionTests a very simple and extensible library
  • Out of the box we could provide a bunch of useful conventions, and people can opt into using them with only a line or two of code
  • No base classes, so I am free to use my own base classes in my project.
  • We can write tests to test our conventions, which you can see above.

What do you think?

@JakeGinnivan
Copy link
Member

Easy enough to remove that, thoughts on the actual changes to the framework?

@mwhelan and I were pairing to just have a play and learn this, and to see if we could separate it from the framework side of things.
Take this PR as a discussion, if you like the way it's going, it needs to be finished properly (and package restore removed :P)

@kkozmic
Copy link
Contributor

kkozmic commented Jul 23, 2013

I only had a quick look at work.

I'll get back to you
On 23/07/2013 5:26 PM, "Jake Ginnivan" notifications@github.com wrote:

Easy enough to remove that, thoughts on the actual changes to the
framework?

@mwhelan https://github.com/mwhelan and I were pairing to just have a
play and learn this, and to see if we could separate it from the framework
side of things.
Take this PR as a discussion, if you like the way it's going, it needs to
be finished properly (and package restore removed :P)


Reply to this email directly or view it on GitHubhttps://github.com//pull/8#issuecomment-21397420
.

@JakeGinnivan
Copy link
Member

Sweet, thanks

This PR would actually fix #4 #5 and #6 I think

@kkozmic
Copy link
Contributor

kkozmic commented Jul 23, 2013

hey, thanks for that guys.

Funny like how just the other day I was emailing @MehdiK about sharing some ideas for v2 of CT and I have a half written email about this in my draft folder. Here's edited part of that email with stuff relevant to this PR. It's long so make sure you have some time put aside to read it :)

I did start making some changes recently for v2 of ConventionTests with a bunch of goals for this release. One of them is breaking the dependency on NUnit. This has been done with IAssert interface and factoring out all the logic that depends on particular test framework. Now __Run.cs (https://github.com/TestStack/ConventionTests/blob/ca8bfe9864bbdb930b3f534ad0205243f2fef1db/ConventionTests/Conventions/__Run.cs) is the only file/type that has any knowledge of NUnit, and similar integration points can be built for other frameworks (xUnit would come as another OOTB supported framework). It also hides/encapsulates all the ApprovalTests integration glue away from the user.

This brings me to my first point. The main vision behind Convention Tests was for it to be a tool for Live Documentation (I may need to come up with a better name for that). Basically the idea is that the ConventionTests could be used as documentation by developers. This idea strongly influenced some design decisions in ConventionTests. Namely:

  • single convention per file - this allows people to just open the Conventions folder and at a glance see what the system is all about. This is especially effective when following the imperative naming convention for the conventions
  • usage of ConventionData - The way the type was built it was also meant to be declarative, simple and quick to use/write conventions against.
  • the usage of ApprovalTests was abstracted away behind a flag so that users could concentrate on what their convention is all about (in a fairly declarative way) rather than imperative details of how to talk to ApprovalTests etc. ApprovalTests was an implementation detail of ConventionTests and I'd rather keep it that way)

The implementation in the first draft of this PR feels to me is losing some of that readability/order. It feels much more low level in way the tests look and by having/forcing no clear distinction/division between "normal" tests and convention tests (and among themselves) makes it harder to discover the conventions in the system and use them as discovery/documentation mechanism.

Now, I can see that's it also has some great advantages.

Tooling integration (as mentioned in #4) is a huge win, and lack of thereof was the main usability drawback of current version. Also not having fixed base classes is a win. To be fair, the old approach didn't strictly require base classes, but it strongly favoured them.

That said, I'd love to hear your feedback and some ideas on how can we better achieve the new improvements, while keeping the readability high.

@JakeGinnivan
Copy link
Member

Hey,

I love the way you think about this stuff.

So our next step was to bring BDDfy and convention tests together with some custom grammar.

This would mean that your conventions are outputted using bddfys reporting :)

Personally i like my convention tests being unit tests, but i usually have them all in a single fixture called 'the_code_base' or CodeBaseConventions.

Being a simple library means the complexity drops and it makes it easier to get started.

We were thinking along the lines of:
this.VerifyConvention()
.ForTypes(....)
.WithExceptions(....)
.Verify();

Verify just throws an exception, if you want approval tests to accept an error, then do:

Var message = this.VerifyConvention()
.ForTypes(....)
.WithExceptions(....)
.VerificationMessage();

Approvals.Verify(message);

But approval tests is optional, i am happy with the exception.

Thoughts?

Sent from my Windows Phone


From: Krzysztof Koźmicmailto:notifications@github.com
Sent: ‎23/‎07/‎2013 11:49
To: TestStack/ConventionTestsmailto:ConventionTests@noreply.github.com
Cc: Jake Ginnivanmailto:Jake.Ginnivan@readify.net
Subject: Re: [ConventionTests] Separation of test framework (#8)

hey, thanks for that guys.

Funny like how just the other day I was emailing @MehdiKhttps://github.com/MehdiK about sharing some ideas for v2 of CT and I have a half written email about this in my draft folder. Here's edited part of that email with stuff relevant to this PR. It's long so make sure you have some time put aside to read it :)

I did start making some changes recently for v2 of ConventionTests with a bunch of goals for this release. One of them is breaking the dependency on NUnit. This has been done with IAssert interface and factoring out all the logic that depends on particular test framework. Now __Run.cs (https://github.com/TestStack/ConventionTests/blob/ca8bfe9864bbdb930b3f534ad0205243f2fef1db/ConventionTests/Conventions/__Run.cs) is the only file/type that has any knowledge of NUnit, and similar integration points can be built for other frameworks (xUnit would come as another OOTB supported framework). It also hides/encapsulates all the ApprovalTests integration glue away from the user.

This brings me to my first point. The main vision behind Convention Tests was for it to be a tool for Live Documentation (I may need to come up with a better name for that). Basically the idea is that the ConventionTests could be used as documentation by developers. This idea strongly influenced some design decisions in ConventionTests. Namely:

  • single convention per file - this allows people to just open the Conventions folder and at a glance see what the system is all about. This is especially effective when following the imperative naming convention for the conventions
  • usage of ConventionData - The way the type was built it was also meant to be declarative, simple and quick to use/write conventions against.
  • the usage of ApprovalTests was abstracted away behind a flag so that users could concentrate on what their convention is all about (in a fairly declarative way) rather than imperative details of how to talk to ApprovalTests etc. ApprovalTests was an implementation detail of ConventionTests and I'd rather keep it that way)

The implementation in the first draft of this PR feels to me is losing some of that readability/order. It feels much more low level in way the tests look and by having/forcing no clear distinction/division between "normal" tests and convention tests (and among themselves) makes it harder to discover the conventions in the system and use them as discovery/documentation mechanism.

Now, I can see that's it also has some great advantages.

Tooling integration (as mentioned in #4#4) is a huge win, and lack of thereof was the main usability drawback of current version. Also not having fixed base classes is a win. To be fair, the old approach didn't strictly require base classes, but it strongly favoured them.

That said, I'd love to hear your feedback and some ideas on how can we better achieve the new improvements, while keeping the readability high.


Reply to this email directly or view it on GitHubhttps://github.com//pull/8#issuecomment-21406243.

@kkozmic
Copy link
Contributor

kkozmic commented Jul 23, 2013

I think I agree with you that moving ConventionTests to a more conventional hosting-testing-framework aligned model.

I also like the idea of having some pre-packaged conventions, although we'll need to pay attention how we do it to strike the right balance between OOTB functionality vs configurability and do it at a right level of abstraction.

That would play nice as generalisation of the WIndsor-specific conventions we had in v1 and if we get the API right and make them easy to build I can see people building other OSS frameworks providing their own CT 'recipes' (or whatever we end up calling them).

Reporting is another BIG thing on my radar. I was thinking about taking it quite far (perhaps not in v2, but in the long run) and use to generate human readabla/searchable documentation (with examples).

As for the API, I'm not sure I like the method-chaining heavy approach. It rubs me the wrong way :)

We're integrating into testing tools ecosystem and those generally follow Static Entry point pattern (yes, I just made that name up).

Assert.Something();
Substitute.For();

As such, I'd see the API evolving in a similar direction, something along the lines (I make this up as I go!)

Convention.Is(new ConventionData {
TheUsualProperties = someValues
});

or for Recipes

Convention.Is(some sort of config here);

Makes sense?

Krzysztof Kozmic

On Tuesday, 23 July 2013 at 9:37 PM, Jake Ginnivan wrote:

Hey,

I love the way you think about this stuff.

So our next step was to bring BDDfy and convention tests together with some custom grammar.

This would mean that your conventions are outputted using bddfys reporting :)

Personally i like my convention tests being unit tests, but i usually have them all in a single fixture called 'the_code_base' or CodeBaseConventions.

Being a simple library means the complexity drops and it makes it easier to get started.

We were thinking along the lines of:
this.VerifyConvention()
.ForTypes(....)
.WithExceptions(....)
.Verify();

Verify just throws an exception, if you want approval tests to accept an error, then do:

Var message = this.VerifyConvention()
.ForTypes(....)
.WithExceptions(....)
.VerificationMessage();

Approvals.Verify(message);

But approval tests is optional, i am happy with the exception.

Thoughts?

Sent from my Windows Phone


From: Krzysztof Koźmicmailto:notifications@github.com
Sent: ‎23/‎07/‎2013 11:49
To: TestStack/ConventionTestsmailto:ConventionTests@noreply.github.com
Cc: Jake Ginnivanmailto:Jake.Ginnivan@readify.net
Subject: Re: [ConventionTests] Separation of test framework (#8)

hey, thanks for that guys.

Funny like how just the other day I was emailing @MehdiKhttps://github.com/MehdiK about sharing some ideas for v2 of CT and I have a half written email about this in my draft folder. Here's edited part of that email with stuff relevant to this PR. It's long so make sure you have some time put aside to read it :)

I did start making some changes recently for v2 of ConventionTests with a bunch of goals for this release. One of them is breaking the dependency on NUnit. This has been done with IAssert interface and factoring out all the logic that depends on particular test framework. Now __Run.cs (https://github.com/TestStack/ConventionTests/blob/ca8bfe9864bbdb930b3f534ad0205243f2fef1db/ConventionTests/Conventions/__Run.cs) is the only file/type that has any knowledge of NUnit, and similar integration points can be built for other frameworks (xUnit would come as another OOTB supported framework). It also hides/encapsulates all the ApprovalTests integration glue away from the user.

This brings me to my first point. The main vision behind Convention Tests was for it to be a tool for Live Documentation (I may need to come up with a better name for that). Basically the idea is that the ConventionTests could be used as documentation by developers. This idea strongly influenced some design decisions in ConventionTests. Namely:

  • single convention per file - this allows people to just open the Conventions folder and at a glance see what the system is all about. This is especially effective when following the imperative naming convention for the conventions
  • usage of ConventionData - The way the type was built it was also meant to be declarative, simple and quick to use/write conventions against.
  • the usage of ApprovalTests was abstracted away behind a flag so that users could concentrate on what their convention is all about (in a fairly declarative way) rather than imperative details of how to talk to ApprovalTests etc. ApprovalTests was an implementation detail of ConventionTests and I'd rather keep it that way)

The implementation in the first draft of this PR feels to me is losing some of that readability/order. It feels much more low level in way the tests look and by having/forcing no clear distinction/division between "normal" tests and convention tests (and among themselves) makes it harder to discover the conventions in the system and use them as discovery/documentation mechanism.

Now, I can see that's it also has some great advantages.

Tooling integration (as mentioned in #4#4) is a huge win, and lack of thereof was the main usability drawback of current version. Also not having fixed base classes is a win. To be fair, the old approach didn't strictly require base classes, but it strongly favoured them.

That said, I'd love to hear your feedback and some ideas on how can we better achieve the new improvements, while keeping the readability high.


Reply to this email directly or view it on GitHubhttps://github.com//pull/8#issuecomment-21406243.


Reply to this email directly or view it on GitHub (#8 (comment)).

@JakeGinnivan
Copy link
Member

Yeah, I like it! Personally i like bddfys reporting.

How did you want to proceed? Maybe pull in the middle commit (separating ConventionTests and the framework stuff), then spike out a new api, including a way to define conventions ootb?

Sent from my Windows Phone

@mwhelan
Copy link
Member Author

mwhelan commented Jul 23, 2013

Interesting to see this API developing throughout the conversation and I think it's getting progressively better. I think I prefer @kkozmic 's static entry point pattern to the fluent chaining approach.

I also like the idea of using BDDfy's reporting. Perhaps something like this

Conventions For: NHibernate
  When applying conventions to domain entities
    [Green success icon] all methods should be virtual
    [Red failure icon] all classes should have a default constructor
        but these classes do not have a default constructor
        - Class1
        - Class2

Conventions For: Castle Windsor
...       

This report doesn't show any failures but gives the general idea of how the styling might look.... Of course we can completely customize it to Convention Tests
http://michael-whelan.net/get/blog_pictures/2013/bddfy-in-action/bddfy-unit-test-report.png

@kkozmic
Copy link
Contributor

kkozmic commented Jul 24, 2013

@JakeGinnivan I'm not sure the 2nd commit on its own is worth pulling in. I'm not convinced splitting this into two (or more) projects is a good idea. Since CT is a code-only package we have a bit more flexibility into how we push it down, and I was thinking of leveraging some powershell smarts in nuget to put it together appropriately. I'll create another issue for that so we can have a more structured discussion around it.

I think we should close off this PR and split the work into the functional areas we were discussing.

@mwhelan I like the idea of leveraging BDDfy for reporting as an option (and it's nice to work well with each other within TestStack ecosystem).

@kkozmic kkozmic closed this Jul 24, 2013
@kkozmic
Copy link
Contributor

kkozmic commented Jul 24, 2013

actually, issue #6 already talks about that idea of being smart and leveraging nuget's contextual awareness to configure the package appropriately to what the user's are using

This was referenced Jul 24, 2013
@JakeGinnivan
Copy link
Member

I think that CT should become a library rather than a code only package.

Reason being is that we will no longer be coupled to the framework hosting us, so the need to make modifications are smaller. Also with the addition of recipes code only is losing it's appeal?

@kkozmic
Copy link
Contributor

kkozmic commented Jul 25, 2013

I'm open to that. It probably makes more sense now, with the direction we're trying to take in v2, than it did when I first started building it.

Krzysztof Kozmic

On Friday, 26 July 2013 at 1:53 AM, Jake Ginnivan wrote:

I think that CT should become a library rather than a code only package.
Reason being is that we will no longer be coupled to the framework hosting us, so the need to make modifications are smaller. Also with the addition of recipes code only is losing it's appeal?


Reply to this email directly or view it on GitHub (#8 (comment)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants