Fix parameter freezing when used in combination with Passive Attributes (#637, xUnit.net2)

[sshushliapin]

Addresses issue #637 for xUnit.net2 Glue Library.

[sshushliapin]

Looking on this change, I can predict a question similar to this one I've been asked earlier. Despite it does solve the original issue, I cannot come up with any self contained use case for the new constructor. I tend to create a new internal version of the FreezeOnMatchCustomization in the Glue Library and move there all the changes from this class.

[ploeh]

Well, yes, sort of. I'm predictable, it seems 😐

Sorry to be a bit negative, but I think this needs to go in a slightly different direction. The overall idea, however, seems sound 👍

Adding a ParameterInfo property pulls in the direction of less cohesion, which I'm not particularly fond of. Now the class has two properties, both of which could be null, or both of which, according to the type system, could have a value. This is the sort of feature extension that tends to rot a code base over time.

Some time ago, @ecampidoglio and I had a discussion about something similar that might be worth revisiting. Essentially, the idea is to widen the existing 'greedy' constructor, so that it takes any object, instead of only a Type. AFAICT, there's nothing in the class's invariants that require it to be a Type.

Given that we now seem to have a use for this change, I think it might be worthwhile pursuing.

[sshushliapin]

Sorry to be a bit negative, but I think this needs to go in a slightly different direction.

That's totally fine. I found a potential fix, we just need to give it a proper shape ;)

I reviewed the discussion you mentioned, thanks for sharing that with me. That is a possible solution but if I understand correctly, the TargetType property and both existing constructors (?) in the FreezeOnMatchCustomization class will need to be obsoleted.

I'm still validating other possible solutions. As a POC, I created a new internal freezing customization. It allows not to change the existing deep-seated API but it brings some other challenges with testing internals.

[ecampidoglio]

Sorry for the late reply but I'm currently on vacation and have only occasional access to the Internet. ☀️🍦

Looking back at the discussion, I noticed that @ploeh did, in fact, predict this exact scenario:

Based on this, I'm inclined to keep it as is. However, I was wondering if we'd then be missing out on some opportunities, like:

new FreezeOnMatchCustomization(somePropertyInfo);
new FreezeOnMatchCustomization(someParameterInfo);
new FreezeOnMatchCustomization(someFieldInfo);
// etc.

Unfortunately, I failed to follow up on that comment so the blame is totally on me. 😔

In the face of this new requirement, I agree that relaxing the constructor of FreezeOnMatchCustomization sounds like the right thing to do.

Of course, we would still have to pay the complexity tax of handling multiple variants of the target parameter. Although it would be nice if we could restrict the subset of types that can be passed as arguments through the type system, in the end, I'm afraid it would still have to be object since there is no other contravariant type between PropertyInfo, ParameterInfo, FieldInfo and Type.

I should mention that I'm saying all this without having researched other possible solutions that don't involve modifying the constructor of FreezeOnMatchCustomization.

[ploeh]

there is no other contravariant type between PropertyInfo, ParameterInfo, FieldInfo and Type.

Well, there's Albedo, which was created to address that particular problem... 😉

[sshushliapin]

Yep. Albedo does the job!
JFI, AutoFixture Core and all the test framework Glue Libraries have to reference it.

[sshushliapin]

The build error looks intimidating:

  1) Building C:\projects\autofixture\Src\AutoFakeItEasy.sln failed with exitcode 1.
  2) CA1506: (-1,0): Microsoft.Maintainability : 'Fixture' is coupled with 81 different types 
from 14 different namespaces. Rewrite or refactor this class's methods to decrease its class 
coupling, or consider moving some of the class's methods to some of the other types it is 
tightly coupled with. A class coupling above 95 indicates poor maintainability, a class 
coupling between 95 and 80 indicates moderate, and a class coupling below 80 indicates 
good maintainability.

Unfortunately I cannot reproduce it locally because of another build error with NUnit3 (even in the latest master branch!):

  1) Fake.UnitTestCommon+FailedTestsException: NUnit test failed. Process finished with 
exit code UnexpectedError (-100).
   at Fake.Testing.NUnit3.NUnit3(FSharpFunc`2 setParams, IEnumerable`1 assemblies) in 
C:\code\fake\src\app\FakeLib\UnitTest\NUnit\NUnit3.fs:line 280
   at FSI_0005.Build.clo@36-6.Invoke(Unit _arg7)
   at Fake.TargetHelper.runSingleTarget(TargetTemplate`1 target) in 
C:\code\fake\src\app\FakeLib\TargetHelper.fs:line 484

[sshushliapin]

2. CA1506: (-1,0): Microsoft.Maintainability : 'Fixture' is coupled with 81 different types

Does that mean that somebody has coupled Fixture "80 times" and I'm the lucky man who has to decouple this? 😨

[sshushliapin]

The issue is caused by the new PropertyInfoElementRelay and TypeElementRelay registrations in the Fixture class. The graph of specimen builders is getting huge.

Moving the graph initialization to another class(es) will just postpone the problem for a while. AutoFixture is designed in such a way that extending the range of supported specimen types often leads to referencing new types in the Fixture class. The classes will continue growing, so it looks like suppressing the error is justified.

[ploeh]

Is this the correct dependency configuration? The actual dependency added is Albedo 1.0.1, but if I understand this configuration correctly, any 1.x version is supported - including 1.0.0.

Additionally, why exclude a hypothetical version 2 of Albedo?

[sshushliapin]

I choose Albedo 1.0.1 simply because it was already referenced in unit tests. I overlooked that the dependency is added to the publicly visible AutoFixture package which should reference the lowest possible version (1.0.0).

I'll get back on this!

[ploeh]

I think it ought to reference the lowest possible version that we know works. If we're using 1.0.1, we should state that as a requirement.

If we wish to be able to state that 1.0.0 is the minimum version supported, we should downgrade our use of Albedo so that we are, in fact, testing against that version.

[sshushliapin]

Everything works fine with Albedo 1.0.0.

BTW, there is AutoFixture.Igioms package which is already published with Albedo v1.0.1 dependency. There should be no harm in downgrading this reference to v1.0.0. Please let me know if you have any concerns about this.

[ploeh]

I think we should have consistent versions for shared dependencies. If AutoFixture.Idioms works with Albedo 1.0.0, I think it'd be more correct to downgrade its dependency as well. I haven't tested, but looking at the change between 1.0.0 and 1.0.1, I think you're correct that it ought to work as well.

[ploeh]

We should probably start with a separate pull request for that.

[ecampidoglio]

@sergeyshushlyapin When it comes to PRs, we generally prefer to have a linear history over merge commits. Could you please remove the merge commits f3c74f9 18fb59d and instead rebase your freezing-behavior branch on top of master?

We – the maintainers – could also do it ourselves once the PR is complete, if you prefer.

[sshushliapin]

No problem, usually I also prefer rebasing over merge commits. I just noticed that rebasing often "collapse" old conversations, so I just tried to preserve them ;)

[ploeh]

Should we deprecate the TargetType property?

[sshushliapin]

It's not used in AutoFixture anymore. Yes, I think it can be deprecated.

[sshushliapin]

This PR changes some core classes (FreezeOnMatchCustomization) as well as Glue Library classes (xUnit2.FrozenAttribute). I started it as xUnit.net2 contribution and intended to fix all the other libraries with follow up requests.

If I deprecate the TargetType property and corresponding constructors (which are in 'core'), then other libraries start failing:

  1) Building C:\sc\autofixture\Src\AutoFixture.NUnit2.sln failed with exitcode 1.
  2) CS0618: FrozenAttribute.cs(113,20): 'FreezeOnMatchCustomization.FreezeOnMatchCustomization(Type, IRequestSpecification)' is obsolete: 'This constructor has been deprecated. Please use constructor that takes IReflectionElement parameter.'

What is the best approach to go? Should I suppress the error in other libraries for the time being or postpone obsoleting the property and constructors?

[sshushliapin]

The property is still valid for other Glue Libraries, so it's too early to make it obsolete.

[ploeh]

It's looking good 👍

The only comment I have is about deprecation of the the TargetType property.

[ecampidoglio]

I apologize if this feedback comes in late, but I have given this some more thought and I'd like to raise a couple of concerns.

By making IReflectionElement part of the public API of FreezeOnMatchCustomization, we're effectively coupling the AutoFixture core API to Albedo from a consumer's point of view. Now, that might not be a problem in and of itself – although I believe the AutoFixture core API has relied solely on the BCL up to this point, while Albedo was used internally.

If we're also going to deprecate the constructor that takes Type as an argument, consumers who wish to use the FreezeOnMatchCustomization in their test projects will now have to take a direct dependency on Albedo. Again, this is not a problem per se but it does smell a little bit of interface pollution, in the sense that it forces consumers to use a library they shouldn't necessarily have to know about (Albedo) in order to take advantage of one of AutoFixture's core features (freezing by criteria). AutoFixture and Albedo solve different problems and, while Albedo is very useful to AutoFixture's implementation, I don't believe their respective APIs belong together.

These are my concerns. What are your thoughts?

[sshushliapin]

From Albedo site:

Albedo was born out of a need for such abstractions in AutoFixture. While it's easy to extract a specific interface to address a specific need, you always run the risk of defining an interface that can only be used in one very specific situation; such an interface may not be a good abstraction.

Won't we end up with Albedo2 inside AutoFixture if we decide to extend the existing core API?

[moodmosaic]

Since some Albedo maintainers are also AutoFixture maintainers, and since it follows Semantic Versioning, it shouldn't be that bad of an idea if we add it as a dependency...

AutoFixture has relied only on BCL, but I can't currently see disadvantages if we'd have to take Albedo as a dependency 😕

[sshushliapin]

One thing worth mentioning (according to my understanding, correct me if I'm wrong) is that the FreezeOnMatchCustomization is not used by AF consumers directly. When one needs to freeze a specimen, he either calls the fixture.Freeze<>() method or use [Frozen] attribute from Glue Libraries.

[ecampidoglio]

We're discussing two different issues here.

@moodmosaic

AutoFixture has relied only on BCL, but I can't currently see disadvantages if we'd have to take Albedo as a dependency

AutoFixture already depends on Albedo. My point is that, up until now, Albedo has been used internally to simplify the implementation of AutoFixture – the consumers don't need to know or care about Albedo in order to use AutoFixture – but with this PR we're effectively surfacing Albedo as part AutoFixture's public API. The practical effect is that whoever needs to use the FreezeOnMatchCustomization will have to take a direct dependency on Albedo.

Now, if Albedo was a namespace within AutoFixture that wouldn't be much of a problem. But since Albedo is currently published as an external general-purpose package, I see a problem in tying it to AutoFixture's public API.

@sergeyshushlyapin

When one needs to freeze a specimen, he either calls the fixture.Freeze<>() method or use [Frozen] attribute from Glue Libraries.

Well, the Freeze method – in its current implementation – doesn't support matching by criteria; if you want to use that feature with the imperative API FreezeOnMatchCustomization is your only option.

On a more philosophical level, I see customizations and specimen builders as AutoFixture's building blocks, combined together to offer functionality that is then conveniently exposed through a higher level API (see the Customize method or the AutoDataAttribute). If a usage scenario isn't directly supported in the API, it should be possible to drop down to the building blocks and combine them in new ways to achieve the goal.

That's why I would rather see that the building blocks themselves continue to be free from dependencies on external libraries.

[sshushliapin]

From Albedo we need just three things: IReflectionElement, TypeElement and ParameterInfoElement. Than might sound a bit impertinently, but what if (as a POC) we just "steal" them and remove the reference?... 😳

[zvirja]

@sergeyshushlyapin
I have concerns regarding the approach you decided to follow. You created custom wrappers and later relays which unpack that wrappers. Such approach leads to the performance overhead (more relays to check, more object are created, extra request is performed during resolve), however the reason behind that is not clear.

Isn't that much simpler to extend the FreezeOnMatchCustomization with object request instead of Type request that customization should freeze? Technically, that will make customization even more generic, because it will be capable of freezing any type of requests.

Another point is extensibility. If later we will need support freezing of another request sources (e.g. PropertyInfo), we will need to create one more wrapper and relay. Eventually, we could end up in wrapper/relay pairs for all type of requests the fixture support. Such perspective doesn't look attractive.

It seems that root of your approach is some kind of semantic meaning, but I don't see it. Given that fixture accepts object as a request, we could use object as a request too.

P.S. Also, usage of object should reduce the complexity of overall freezing logic and number of changes applied above :)

P.P.S. Excuse me, if I missed something obvious.

[sshushliapin]

Such approach leads to the performance overhead (more relays to check, more object are created, extra request is performed during resolve)

I don't think that is the case. Relays are widely used in AutoFixture and according to test execution results there are no performance issues.

Isn't that much simpler to extend the FreezeOnMatchCustomization with object request instead of Type request that customization should freeze?

That is one of the three mostly discussed options:

• Use objects
• Introduce new abstraction
• Rely on Albedo

If later we will need support freezing of another request sources (e.g. PropertyInfo), we will need to create one more wrapper and relay.

If we talk about Albedo than yes, we will. But are such scenarios numerous?

Given that fixture accepts object as a request, we could use object as a request too.

Yep. The object parameter does fit the existing design (which is "quite weakly typed").

[ploeh]

The value proposition offered by Albedo is that it enables a client to model a variety of Reflection types in such a way that they look like they have a common consumption interface: IReflectionVisitor<T>. This enables a client to implement the interface. When visited by the various heterogeneous Reflection types, no type tests are required because of the double dispatch design.

This also forces a client (developer) to consider how to deal with all of the concrete IReflectionElement subtypes. This adds a degree of type safety to implementations.

None of these features are in play in this pull request. Instead, the two proposed relays perform type tests and use the downcast objects to access the underlying Reflection objects. In this case, I can't see that wrapping Type in TypeElement and ParameterInfo in ParameterInfoElement adds any value. We could achieve the same functionality by simply requesting Type and ParameterInfo.

Thus, I don't see that this particular use of Albedo adds any value, and thus I don't think that the added dependency is warranted.

In order to prevent more thrashing on this particular PR branch, I'm going to close it. @sergeyshushlyapin, as the owner of the fork, you still have the commits, so you should be able to salvage the good parts of this PR from the commits that introduce Albedo, so I'm not saying that everything in this PR is useless. Quite the contrary, and I'd welcome a second attempt, but I think it ought to be in a fresh PR.

---

I do realise that if anyone is responsible for introducing Albedo to this work item, it's me:

"Well, there's Albedo, which was created to address that particular problem..."

I wrote this in response to the assertion that there's no polymorphic type that can model both PropertyInfo, ParameterInfo, FieldInfo and Type. This is a problem that haunts much of the AutoFixture code base. At a certain time, it became so bad that we created Albedo to address that problem. Albedo is, in fact, a dependency of AutoFixture.Idioms, although not of AutoFixture itself.

The thought of making AutoFixture itself depend on Albedo is not foreign to me. If it's warranted, I'd be happy to entertain the idea (while still listening to arguments against the notion).

I'd consider it warranted if it simplifies the user-aimed API of AutoFixture.