`ShouldBeOfType/ShouldBe` should test for type equality instead of "type or derived type"

[ulrichb]

At the moment ShouldBeOfType is implemented this way:

public static void ShouldBeOfType(this object actual, Type expected)
{
  if (actual == null)
    throw new SpecificationException ...

  if (!expected.IsAssignableFrom(actual.GetType()))
    throw new SpecificationException ...
}

Which is especially bad for specs like ...

It should_not_allow_the_transfer = 
  () => exception.ShouldBeOfType<Exception>();

... because this spec would succeed for a thrown IOException too.

I think it would be better to use type equality:

  if (expected != actual.GetType())
    throw new SpecificationException ...

This would be of course a breaking change, which I think would be OK, because a) we are pre-1.0 and b) this would be the expected behavior of ShouldBeOfType.

We could add an additional ShouldBeAssignableTo (or ShouldBeOfTypeOrDerivedFrom) to provide the old behavior too (and a simple upgrade path).

Furthermore this would solve the inconsistency with ShouldNotBeOfType, which does already an equality-compare.

[ulrichb]

BTW, this is the behavior of NUnit ...

[Test]
public void Is_InstanceOf()
{
  Assert.That("string", Is.InstanceOf<object>()); // works
}

[Test]
public void Is_TypeOf()
{
  Assert.That("string", Is.TypeOf<object>()); // fails => IsType does an exact match
}

.... and this is the behavior of xUnit...

[Fact]
public void IsAssignableFrom()
{
  Assert.IsAssignableFrom<object>("string"); // works
}

[Fact]
public void IsType()
{
  Assert.IsType<object>("string"); // fails => IsType does an exact match
}   

So "by default" (TypeOf and IsType) they perform an exact type match, and they call the "type or derived type" assertions InstanceOf and IsAssignableFrom.

[danielmarbach]

No we are in trouble. If we change the underlying implementation to match the ones of NUnit and Xunit we potentially break tests because people might rely on the specific behavior of an assertion (the bug has become a feature). Any suggestions how we handle that gracefully?

ulrichb notifications@github.com hat am 8. November 2013 um 18:46 geschrieben:

BTW, this is the behavior of NUnit ...

[Test]
public void Is_InstanceOf()
{
   Assert.That("string", Is.InstanceOf<object>()); // works
}

[Test]
public void Is_TypeOf()
{
   Assert.That("string", Is.TypeOf<object>()); // fails => IsType does an exact
match
}

.... and this is the behavior of xUnit...

[Fact]
public void IsAssignableFrom()
{
   Assert.IsAssignableFrom<object>("string"); // works
}

[Fact]
public void IsType()
{
   Assert.IsType<object>("string"); // fails => IsType does an exact match
} 

So "by default" (TypeOf and IsType) they perform an exact type match, and they call the "type or derived type" assertions InstanceOf and IsAssignableFrom.

---

Reply to this email directly or view it on GitHub: https://github.com/machine/machine.specifications/issues/171#issuecomment-28082504

[ulrichb]

@danielmarbach Yes, of course this would be a breaking change, and I tried to argue in the issue description why we should still go this way. The comparison with NUnit/xUnit is just one more argument.

Regarding "how we handle that gracefully": We could provide a new method with the "type or derived type" behavior (see issue description) and document this in the release notes.

[NameOfTheDragon]

I must admit I have been caught off guard by this, although once I had read the code and figured out what was going on, I sort of got used to it and now I kind of like the existing behaviour – I don’t want all my tests fracturing because I subclassed something, or returned a different interface implementation. So I’m coming down on the side of preserving the status quo. I think existing behaviour should be preserved now, regardless of what *unit does. I know the version is currently ‘pre-1.0’ but there is a whole other debate about whether that is the right situation for MSpec to be in. I suspect many users regard it as a stable, released product and are using it ‘in anger’ to test production code – I know I am.

I suppose a counter argument might go something like… “The Liskov Substitution Principle says that a subtype should be substitutable for a parent type and behave the same way; since MSpec is a BDD framework then testing for same behaviour rather than type equality is the right thing to do”. Yes? Maybe. I don’t know. I’m not a BDD expert by a long way and I’m not sure how reasonable a statement that is, but I’m just trying to see this from all the different perspectives.

I suppose it’s not the end of the world if I have to change a few assertions, but I’d rather preserve the status quo and introduce a new assertion called something like ShouldBeOfExactType() – it would show up in Intellisense right next to ShouldBeOfType and it would be pretty clear what is going on, methinks.

--Tim Long

From: ulrichb [mailto:notifications@github.com] Sent: 04 October 2013 15:47 To: machine/machine.specifications Subject: [machine.specifications] ShouldBeOfType/ShouldBe should test for type equality instead of 'IsAssignableFrom' (#171)

At the moment ShouldBeOfType is implemented this way:

public static void ShouldBeOfType(this object actual, Type expected)

{

if (actual == null)

throw new SpecificationException ...

if (!expected.IsAssignableFrom(actual.GetType()))

throw new SpecificationException ...

}

Which is especially bad for specs like ...

It should_not_allow_the_transfer =

() => exception.ShouldBeOfType();

... because this spec would succeed for a thrown IOException too.

I think it would be better to use type equality:

if (expected != actual.GetType())

throw new SpecificationException ...

This would be of course a breaking change, which I think would be OK, because a) we are pre-1.0 and b) this would be the expected behavior of ShouldBeOfType.

We could add an additional ShouldBeAssignableFrom (or ShouldBeOfTypeOrInheritedFrom) to provide the old behavior too (and a simple upgrade path).

Furthermore this would solve the inconsistency with ShouldNotBeOfType, which does already an equality-compare.

— Reply to this email directly or view it on GitHubhttps://github.com/machine/machine.specifications/issues/171.

ExchangeDefender Message Security: Click below to verify authenticity http://www.exchangedefender.com/verify.asp?id=rA8IdLeX002617&from=tim@tigranetworks.co.uk Complete email hygiene and business continuity solution available from http://www.tigranetworks.co.uk

[danielmarbach]

Would you be willing to provide a pull against develop?

I think it is Ok because in this release I also pulled out Should Extensions into its own library. So we can also do this change now.

ulrichb notifications@github.com hat am 8. November 2013 um 19:34 geschrieben:

@danielmarbach Yes, of course this would be a breaking change, and I tried to argue in the issue description why we should still go this way. The comparison with NUnit/xUnit is just one more argument.

Regarding "how we handle that gracefully": We could provide a new method with the "type or derived type" behavior (see issue description) and document this in the release notes.

---

Reply to this email directly or view it on GitHub: https://github.com/machine/machine.specifications/issues/171#issuecomment-28086078

[danielmarbach]

Thanks Tim! I like your reasoning! I'm also for preserving the existing behavior and adding a new should extension.

Am 08.11.2013 um 19:39 schrieb NameOfTheDragon notifications@github.com:

I must admit I have been caught off guard by this, although once I had read the code and figured out what was going on, I sort of got used to it and now I kind of like the existing behaviour – I don’t want all my tests fracturing because I subclassed something, or returned a different interface implementation. So I’m coming down on the side of preserving the status quo. I think existing behaviour should be preserved now, regardless of what *unit does. I know the version is currently ‘pre-1.0’ but there is a whole other debate about whether that is the right situation for MSpec to be in. I suspect many users regard it as a stable, released product and are using it ‘in anger’ to test production code – I know I am.

I suppose a counter argument might go something like… “The Liskov Substitution Principle says that a subtype should be substitutable for a parent type and behave the same way; since MSpec is a BDD framework then testing for same behaviour rather than type equality is the right thing to do”. Yes? Maybe. I don’t know. I’m not a BDD expert by a long way and I’m not sure how reasonable a statement that is, but I’m just trying to see this from all the different perspectives.

I suppose it’s not the end of the world if I have to change a few assertions, but I’d rather preserve the status quo and introduce a new assertion called something like ShouldBeOfExactType() – it would show up in Intellisense right next to ShouldBeOfType and it would be pretty clear what is going on, methinks.

--Tim Long

From: ulrichb [mailto:notifications@github.com] Sent: 04 October 2013 15:47 To: machine/machine.specifications Subject: [machine.specifications] ShouldBeOfType/ShouldBe should test for type equality instead of 'IsAssignableFrom' (#171)

At the moment ShouldBeOfType is implemented this way:

public static void ShouldBeOfType(this object actual, Type expected)

{

if (actual == null)

throw new SpecificationException ...

if (!expected.IsAssignableFrom(actual.GetType()))

throw new SpecificationException ...

}

Which is especially bad for specs like ...

It should_not_allow_the_transfer =

() => exception.ShouldBeOfType();

... because this spec would succeed for a thrown IOException too.

I think it would be better to use type equality:

if (expected != actual.GetType())

throw new SpecificationException ...

This would be of course a breaking change, which I think would be OK, because a) we are pre-1.0 and b) this would be the expected behavior of ShouldBeOfType.

We could add an additional ShouldBeAssignableFrom (or ShouldBeOfTypeOrInheritedFrom) to provide the old behavior too (and a simple upgrade path).

Furthermore this would solve the inconsistency with ShouldNotBeOfType, which does already an equality-compare.

— Reply to this email directly or view it on GitHubhttps://github.com/machine/machine.specifications/issues/171.

ExchangeDefender Message Security: Click below to verify authenticity http://www.exchangedefender.com/verify.asp?id=rA8IdLeX002617&from=tim@tigranetworks.co.uk Complete email hygiene and business continuity solution available from http://www.tigranetworks.co.uk

— Reply to this email directly or view it on GitHub.

[ulrichb]

@NameOfTheDragon One example of tests where it is important to test for type equality is an assertion for a thrown exception (because you have contracts like: if state x is violated, an exception of type BaseException will be thrown). Why is the exact match important in this case? Think of a consumer like this:

try { ThrowingMethod(); }
catch (DerivedException) { HandleStateYIsViolated(); }
catch (BaseException) { HandleStateXIsViolated(); }

Then it makes a difference if DerivedException, or BaseException is thrown if state x is violated.

Another example would be a test for a factory, repository, ... (e.g. I want to assert that the product (query result, ...) is an Employee and not a Manager: Employee).

BTW, I think the LSP does not apply here, because in this case I want to test the behavior of the throwing method resp. the factory and not the behavior of the products (which should of course respect the LSP).

For both examples, assertions like Ex.ShouldBeOfType<BaseException>() resp. Product.ShouldBeOfType<Employee>() would not detect a thrown DerivedException resp. produced Manager.

Another dangerous consequence concerns regression testing: for example originally your method throws Exception (and your test asserts ShouldBeOfType<Exception>) and then you refactor your method to throw an ArgumentException for the same state instead. Your test will not show up the changed behavior.

Can you provide examples where the current "type or derived type"-behavior of ShouldBeOfType is really the tool of choice? One use-case that comes to my mind are It-statements in a reusable Behavior-class.

[danielmarbach]

So what do we do with that issue when you both a different opinions. I currently do not have much focus for the should extensions. Would it be an option for you guys to do a g+ hangout or skype session, come to an agreement and then come up with a strategy (if breaking changes are necessary) and send the necessary pulls to develop?

[NameOfTheDragon]

I don't feel strongly either way, really. It would be a breaking change and that may catch a few people out, the only point I'm making is to make sure the benefit is worth the risk. If you think the change is justified then I have no objection. There's nothing to stop people writing their own extensions to get the behaviour they want.

[danielmarbach]

@ulrichb after having slept over it several times I decided to fix it. I will look into it until the end of the week and see if I can come up with a good way for the users

Am 15.01.2014 um 03:11 schrieb Tim Long notifications@github.com:

I don't feel strongly either way, really. It would be a breaking change and that may catch a few people out, the only point I'm making is to make sure the benefit is worth the risk. If you think the change is justified then I have no objection. There's nothing to stop people writing their own extensions to get the behaviour they want.

— Reply to this email directly or view it on GitHub.

[fschmied]

FWIW, I'm for the breaking change because I think:

• it's less surprising to me that ShouldBeOfType performs an exact match than that it performs an assignability match (possibly because I know other frameworks),
• the breaking change is explicit (i.e., it creates red tests after upgrading rather than silently doing the wrong thing), and
• code relying on the old behavior is easily fixed (you can search/replace if you want the old behavior).

BTW, an option to make the breaking change even more explicit would be to mark ShouldBeOfType [Obsolete] (with an explanatory message) and instead add ShouldBeOfExactType and ShouldBeAssignableTo.

[fschmied]

@danielmarbach Sorry, missed your previous answer.

[NameOfTheDragon]

On reflection, I’m happy to go along with that. I particularly like the suggestion to mark ShouldBeOfType as [Deprecated] and introducing two new methods with explicit meaning. --Tim

From: fschmied [mailto:notifications@github.com] Sent: 15 January 2014 10:35 To: machine/machine.specifications Cc: Tim Long Subject: Re: [machine.specifications] ShouldBeOfType/ShouldBe should test for type equality instead of "type or derived type" (#171)

FWIW, I'm for the breaking change because I think:

• it's less surprising to me that ShouldBeOfType performs an exact match than that it performs an assignability match (possibly because I know other frameworks),
• the breaking change is explicit (i.e., it creates red tests after upgrading rather than silently doing the wrong thing), and
• code relying on the old behavior is easily fixed (you can search/replace if you want the old behavior).

BTW, an option to make the breaking change even more explicit would be to mark ShouldBeOfType [Obsolete](with an explanatory message) and instead add ShouldBeOfExactType and ShouldBeAssignableTo.

— Reply to this email directly or view it on GitHubhttps://github.com/machine/machine.specifications/issues/171#issuecomment-32350225.

ExchangeDefender Message Security: Click below to verify authenticity http://www.exchangedefender.com/verify.asp?id=s0GH6Kiv032481&from=tim@tigranetworks.co.uk Complete email hygiene and business continuity solution available from http://www.tigranetworks.co.uk

[danielmarbach]

@fschmied and @NameOfTheDragon could you guys try latest unstable release and also commit review my checkin. I'll close this issue. Feel free to reopen if necessary