Initial value of out parameters is not ignored when asserting calls

[Dashue]

I tend to use Try apis alot, that returns a bool about the succes and the result as an out parameter.

I have trouble asserting calls to these and i end up either skipping the assert or using WithAnyArguments.

I would like a way (could be a variant of WithAnyArguments) which only ignores the out parameters.

If i have a method MyMethod(int x, int y, out long z)
And A.CallTo(() => MyMethod(1, 2, out ignore)).MustHaveHappened.`

Then if x and y is something else than 1 and 2 it should fail, but at the same time ignore the value of the out parameter.

Like IgnoreOutParametersInitialValue(). Behaving somewhat similar to WithAnyArguments but inspecting what type of parameter it is.

Thoughts?
I wouldn't mind contributing this if it's something that can be of value.
Cheers

[blairconrad]

Hi, @Dashue. Thanks for bringing this up. I like the idea of somehow not constraining on out parameter values.

Ultimately, we'll need to wrangle back and forth over the exact syntax/API. Could you provide a full example of what you would like to see (not implementation). For example, I can't tell where the IgnoreOutParametersInitialValue() should show up.

I have additional things to consider:

1. ref parameters may behave similarly, at least in that it's hard to ignore them. Do you have plans for looking at them as well?
2. As you pointed out to me, back in the day there was an issue about this on Google Code. At that time the idea of always ignoring the input for the out parameters was floated, and @patrik-hagne was against it. At the risk of restarting something, I'm kind of in favour of it. After all, methods can't access the values of their out parameters anyhow, so what value is there in performing argument constraint matching on them? Mr. Hägne, I don't suppose your stance has softened?

[blairconrad]

Actually, I'm recalling a comment that @adamralph made not too long ago, about the possibility of having A<T>.Ignored be a field instead of a property. It would mean introducing special logic to identify the Ignored and build a constraint at expression call rule interpreting time, but if it worked out, I think it has the best potential to solve the out and ref problem without introducing special syntax. I haven't followed the code all the way down, but wanted to mention the approach as a possibility.

[Dashue]

I will go ahead and bump with the purpose of seeing if people still feel the same about it, and if so, to figure out a path to proceed along.

[crutt]

This is something I hit recently as well and would love to see some way to handle this situation. The proposed A<T>.Ignored is the type of behavior I expected to be able to use, and if it doesn't have negative impact elsewhere I think it would be a good solution.

[patrik-hagne]

I'm not really in favour of changing the current Ignored-property.

@FakeItEasy/owners what do you think of the following? I've not tried to implement any of it and there is definitely room for improvement in the wording and so forth, but I think it might work:

A.RefIgnored<string>().Leaves().Arg

A.RefIgnored<int>().AssignsLazily(x => 10).Arg

A.Ref(A<int>.That.IsGreaterThan(3)).Leaves().Arg

A.Ref(3).Leaves().Arg

A.Ref(A<string>.That.StartsWith("foo")).AssignsLazily(x => "bar").Arg


public static RefParameterSpecification<T> RefIgnored<T>()
{

}

public static RefParameterSpecification<T> Ref<T>(T valueOrConstraint)
{

}

public static IRefParameterSpecificationBuilder<T> Ref<T>(T valueOrConstraint)
{

}

public static IRefParameterSpecificationBuilder<T> RefIgnored<T>()
{

}

public interface IRefParameterSpecificationBuilder<T>
{
    RefParameterSpecification<T> Leaves();


    RefParameterSpecification<T> AssignsLazily(Func<T, T> assignment);
}

public struct RefParameterSpecification<T>
{
    public T Arg;
}

[blairconrad]

Very interesting, @patrik-hagne. As you say, the language could use some tweaking (I'm not sure "Leaves" will mean much to people right off), but generally I like it!
One thing that frightens me is the interaction between .AssignsLazily and .AssignsOutAndRefParameters(Lazily). I think I like specifying the assignment right on the parameter, though…

[Dashue]

Trying to wrap my head around why the input value of an Out would be considered, and that opt-in to ignore is the default and not opt-out.
To be it seems the default is wrong, I'm sure I don't have the full picture so please bear with me :)

This comment touches more upon why we need a solution than actually help finding the solution, sorry bout that.

[Dashue]

I have no further feedback on this, I will wait on the higher powers to decide and then see if I can chip in some labor : )

[patrik-hagne]

I'd say that the main reason for the input value to be considered is that it's not always out-parameters, it's used for ref-parameters as well.

[Dashue]

Ah I see. I think I remember @blairconrad telling me that ref and out are treated / handled the same. Is this the case? And if so would it be a good idea to investigate a split of handling ref and out? I can understand the initial value being of necessity for ref.

[blairconrad]

@Dashue, the ref and out parameters (and all the "normal" parameters too) are handled the same way - a constraint is built from the expression in the CallTo. For out and refs, the expression will always be an assignable value (a "variable", as I like to call them in my non-precise way), so the constraint that's built is an equality matcher.

And I agree with you about special casing this. I'm still of the opinion that using an always-true constraint for out parameters is a good idea.
It still leaves the problems with refs, but we'd at least be able to provide value right away, and with no special syntax. Just all of a sudden, out parameters do what (some people think) they should.

[blairconrad]

Hmm. I may have accidentally implemented value-ignoring out parameters. While looking around to get enough info to make my last comment, I saw that it would be easy to check the outness of an argument in ExpressionArgumentConstraintFactory.GetArgumentConstraint and generate an IArgumentConstraint that always passes. Perhaps it's not the best way, but it seems to work…

[patrik-hagne]

I'm probably in favor of ignoring the input value of out-parameters as well. Keep in mind that it is a breaking change though.

[blairconrad]

Bah. Usually I'm the one crying "breaking change". I missed my opportunity. You're right, of course. Now, I'd say that actually relying on the constraints that FakeItEasy applies to out parameters is, I would argue, a programming error, but you are right, someone might be adversely affected.

[Dashue]

So this brings an interesting challenge. Can we and do we want to make this a non-breaking change? Or maybe we bite the bullet because it's currently being handled "not the right way"?
Does FIE have any notion of configuration, like could this be something we can configure, and default to the old behaviour (although broken)?

[blairconrad]

It would be possible to configure this behaviour using the Bootstrapper, but I'm not wild about the idea, mostly for two reasons.

1. configuration sucks
2. the current behaviour is just not right. I'd rather force people off it

Of course, we could introduce another mechanism, such as the ones @patrik-hagne outlined, to allow people to start ignoring initial values for ref parameters, but for the out case, I think it's just the way FakeItEasy should work.

As far as "the breaking change" aspect goes, since we're breaking something that was not even so good, I'd tend to worry a little less about it. However, I can appreciate the argument that SemVer says we should go to 2.0.0 or something if we make the change. Which I'd do, but I think I'm more willing to bump major versions than some.

[Dashue]

I wonder.. It's not really breaking the api. I'm not too familiar with semantic versioning details, but it's semver related to the public api? For me, I could easily consider it a small bugfix that fixes incorrect behaviour.

Just as a thought experiment, can someone think of a scenario, or produce one in code, that would show this as being a real issue? I.e you have a test, and that breaks by not considering the original value of out values. I cannot think of anything that doesn't involve some major incorrect assertions (if even possible). I know you are way smarter than me, maybe you can come up with something?

[blairconrad]

"way smarter" 😆

Thanks. I needed a good chuckle.

I'm a little fuzzy on the SemVer stuff myself. I've seen others argue that it's only "breaking" if signatures change, or something of that nature, but I think of it as referring to behaviour, so even if things keep "linking" and whatnot, if the thing acts differently, it would be "breaking".

I agree that the proposed change is fixing incorrect behaviour. However, I've been trained (and I think @patrik-hagne leans this way too) that no matter what horrible behaviour your app or library has, it's some user's favourite feature! Hmm. How could this hurt someone? I think it's less about causing a passing test to fail than it would be to accidentally cause an existing test that should fail when production code changes to not fail. That was confusing. Stay with me.

Someone's been using a variable for… something. I dunno. Maybe they're populating it, in a loop, using a TryGet kind of operation. They've structured their tests so their A.CallTos happen to be validating the old state, before the TryGet was called. Now we always pass and they'll never know that their changed logic has broken things.

Of course, I think that's a terrible test, and that the client would be better off checking those values some other way—when they get used as inputs to collaborators or when the method returns the array of accumulated values or something. But if that's what's been working for them up 'til now, it would "break" their tests.

I guess it feels a little different than just a bug fix because the existing behaviour would've forced users to change their argument constraints to match the values that were present when the out-having method was called, and so we've been telling them "this MustHaveHappened (or whatever) will guarantee that your arguments were exactly what you said".

I feel a bit like a hypocrite (or maybe Devil's Advocate) here by saying all this, though, because I'd really like to slip in the "ignore the outs" change, and if FakeItEasy were my project, I might just do it. 😊
And I think if we did, very few people would be negatively affected, and those that were could probably adapt quicly. Still, upping the major version to accompany such a change would be fair warning to them.

[blairconrad]

@FakeItEasy/owners, I'm in favour of a change to have out values ignored in constraints, without an API change. I think it would provide the most long-term value for FIE clients.
@patrik-hagne has pointed out the breaking nature of the change, so I can move the issue to the 2.0 milestone, if nobody objects. I'd also be inclined to start a conversation about when to make the 2.0 release, but this is not the place.

[philippdolder]

I agree with changing the behavior without API-changes as well as bumping the Major version

[blairconrad]

I've pushed my implementation to blairconrad/FakeItEasy@5d6f674. I didn't want to initiate a PR until we've all weighed in on jumping to 2.0.0 for/with this.

[blairconrad]

Repushed to blairconrad/FakeItEasy@2d7144f.

[adamralph]

OK, I've read back through the comments and I agree that we should change the behaviour to ignore the initial arguments passed to out parameters.

However, I am in favour of not bumping the major version. The initial arguments supplied to out parameters have no value since they are inaccessible, so yes, whilst MustHaveHappened() is ensuring the call was made with the initial values specified, this assertion has no value whatsoever. If the arguments were to change it would break current tests but it could not possibly impact production code. For this reason, FakeItEasy should never have been inspecting the initial arguments to out parameters and doing so is a bug, which makes this a bug fix.

This is my opinion, but I won't hold up things if others want to publicise it as a breaking change and bump the major version. It won't do any harm, even if I feel it's redundant.

As usual, I may be missing something, yada yada, so please correct me if so.

[adamralph]

Oh, and just the usual gentle reminder that any change can break someone including all those that have already gone in since 1.0 😉.

[blairconrad]

I'm actually mostly on the same page as you for this one. Well, I'm of two minds.

I agree that relying on catching differences in the initial value means there's a bad test, so the writer gets what they deserve. And that releasing this issue without bumping major version is analogous to what we did with #268.

The other other part of me says, "yes, but that's cold comfort to the writers who were relying on it and all of sudden have failing tests"—it's not quite the same as never having looked at the values.
And I'd hate to spend whatever good reputation FakeItEasy has on such a problem. At least if the new version breaks things, with a major number jump we can say, "Look. You were warned."
And if it doesn't break things, as you say, what's the harm?

[adamralph]

I'm continuining this thread on academic grounds only - I don't want to hold up this issue and we should just press ahead with 2.0 if that's what the team wants to do.

Given that breaking is necessarily subjective, the challenge posed is to have common agreement on what constitutes breaking. I propose that the percentage of impacted users is the best measure. For this change, I would argue that a tiny minority will be relying on the current bad behaviour and it is they who would be broken by this change. Again, only their tests will fail, their production code will not be impacted. Moreover, their tests are flawed and this 'bug fix' would expose the flaw in their tests and force them to fix it. So I would argue rather than breaking them, we are fixing them 😉.

Regarding the 2.0 bump, as I see it, the intention of SemVer here is to warn users not to upgrade unless they are prepared to do some work to make their code work with the new version. What users should be doing is restricting their dependency to less than 2.0 so that automatic upgrades are guaranteed not to break. However, this is not what NuGet does when adding a dependency and I wager 99% of users do not manually edit their packages.config in this way. If they did, a major version bump would have a clear downside since it would slow adoption of the new version. I believe what really happens is that when people see 2.0 they get excited about the version bump since it is embedded in the psyche that it means 'new and shiny' rather than 'warning - hold off unless you are sure' and they may even be more likely to upgrade sooner which means a 2.0 bump may actually speed up adoption of the new version. I guess the only risk here is disappointment when they poke around in 2.0 after upgrading and see no or very few new features or even just a single feature or bug fix which is of minority interest but has been deemed to be breaking. Again, this is only of academic interest in the context of this issue (although if it changes minds then so be it), but interesting nonetheless 😉.

[blairconrad]

I don't think you're holding the issue up. If nothing else, I wouldn't mind throwing a fix for #338 in the next release along with this, and that'll take another day or two anyhow.

On to other points. I agree that in this case, we are forcing users relying on the current behaviour to fix their tests. Still, we are forcing them.

I'm not comfortable arguing that only users tests will fail, not their production code, since we're a mocking framework and are unlikely (I hope) to be used in production code. Or another way of looking at it: from our perspective, the tests are the production code. The argument about them being flawed to begin with holds a lot of weight, though. Enough that after all this thinking, I'd be content including this issue in a bug fix release.

To continue being academic: I had wondered about SemVer + NuGet. If the majority of users will automatically get the 2.0.0 version when upgrading anyhow, the benefits of SemVer plummet. Then again, I'm not sure how often people upgrade their packages. I rarely do, unless I think there's some new feature or fixed but (that's been troubling me). Excellent point about the major bump scaring people off, although I'd've hoped that users paying attention to that would also pay attention to the release notes and say "Oh, that sounds fine."

As far as "major == shiny" being embedded in the psyche, if we refuse to bump the major version because we don't have shiny new features, we're just reinforcing that perception. If we believe in SemVer, I'd suggest trying to walk the walk, and I would hate to think that we'd let "breaking" changes pile up as we wait for something sufficiently shiny to come along.

The more I type, the more I'm coming 'round. In this case, we're fixing bad FakeItEasy behaviour. I think we can look at this as a bug fix, and not require the major version bump. Of course someone may have discovered the bug and be relying on it. I'd said

no matter what horrible behaviour your app or library has, it's some user's favourite feature

but that doesn't mean we have to preserve the bug, and a wise man once said

any change can break someone

This change should only affect

• a very small set of users,
• who were relying on a bug
• to write "bad" tests

Okay. I'm in the "bump the third component, not the first" camp now. @philippdolder, @patrik-hagne, do you want to talk us out of it?

[blairconrad]

Rebranding as a bug, according to recent discussion and improved understanding of the behaviour.

[blairconrad]

Holding onto this for a bit until we have a quick chat over at #220. I'm hopeful for a speedy resolution of that, and then we may still be able to get this into milestone 1.23.0.

[blairconrad]

Moved ref discussion to #18.

[blairconrad]

@Dashue, thanks very much for your work on this issue. Look for your name in the release notes. 🏆

This issue has been fixed in release 1.23.0.

https://www.nuget.org/packages/FakeItEasy/1.23.0
https://github.com/FakeItEasy/FakeItEasy/releases/tag/1.23.0