Deprecate ClearConfiguration

[blairconrad]

As discussed in #1764, it's somewhat confusing, and untangling it is probably not worth the effort. The preferred method of getting a Fake with no configuration is to just make a new one.

[adamralph]

Out of interest, why is deprecation breaking?

[adamralph]

FTR most of the previous deprecations are not labelled as breaking.

[thomaslevesque]

Hi @adamralph, nice to see you here!
IIRC, we decided to mark deprecations as breaking because they will cause warnings that weren't there before. If you have "warnings as errors" enabled, updating to the new version will cause the build to fail.

[thomaslevesque]

(we started applying this rule for 5.0.0, I think, which is why earlier deprecations weren't labelled as breaking)

[adamralph]

hey @thomaslevesque, thanks for the explanation.

FWIW, IMO those changes are not breaking. If someone has enabled "treat warnings as errors", they are effectively asking the SDK to break their build on non-breaking changes. The messages are still, semantically, warnings. Ultimately, a consumer could configure their build to break in reaction to any type of change, and you have no control over that. The only thing you have control over of is the semantics of the change you are making, and in the case of adding [Obsolete("...")], which is effectively [Obsolete("...", false)], you are converying the fact that this is not breaking with the second argument. There is a way to convey breakage, and that is to pass true for the second parameter.

FWIW, the approach we use for NServiceBus is:

• obsolete member with false in minor+1 and leave the body of the member unchanged - this informs the user that the member is going to be removed in major+1, so they have a chance to change their code now, but allows them to keep using the member until they have time to do that
• obsolete member with true in major+1, and remove the body of the member - this forces the user to change their code now, since it is a breaking change, and since the code will not compile while the member is used, the body of the member serves no purpose, hence its removal
• remove member entirely in major+2 - all users would have removed their usages by now, so the member no longer serves any purpose

Note that we also combine this with a recommendation to upgrade one major version at a time, which allows the messages of the obsolete attributes to guide them.

[thomaslevesque]

Hi @adamralph,

I see your point. Your approach in NServiceBus is interesting. You effectively remove the member at the same point as we do (major + 2), but you start warning the user earlier, which I think is good. With our approach, we go directly from "warning that the user could ignore" to "remove the method completely", which leaves the user wondering what to do about it. I like the idea of "obsolete with true", which forces the user to take action, and still gives information about what to do (assuming the error message says what to use instead).

@blairconrad what do you think?

[blairconrad]

Hey, @adamralph. Long time no see!

I think @thomaslevesque has the reasoning right. We didn't want to excessively bother people who had "treat warnings as errors" on. @adamralph, your point ("they literally asked for it", if I may paraphrase) is valid.

I'm not fussy. I think the current approach is fine, but the NServiceBus one also seems like a good one. I'm okay to adopt it. If we do, I'd consider documenting the what and why so we have a record. Issue labels seems like the best existing wiki page, if we didn't want to create a new one…

[adamralph]

There's also something in SemVer about this:

How should I handle deprecating functionality?

Deprecating existing functionality is a normal part of software development and is often required to make forward progress. When you deprecate part of your public API, you should do two things: (1) update your documentation to let users know about the change, (2) issue a new minor release with the deprecation in place. Before you completely remove the functionality in a new major release there should be at least one minor release that contains the deprecation so that users can smoothly transition to the new API.

[blairconrad]

I'm fine with a change, including to the NServiceBus model. Glad to see it's consistent with SemVer's recommendation, but keep in mind, FIE's current approach is as well. We do document, issue a new minor with the deprecation in place (and a major), and finally a new major with the functionality removed.

[adamralph]

We do document, issue a new minor with the deprecation in place...

Perhaps I'm misunderstanding then. If all your deprecations are treated as breaking, then you only release them in majors, no?

[blairconrad]

I believe that in practice there's always a new minor before the major.

I'm not arguing against a switch to the model followed by NServiceBus.

[adamralph]

@blairconrad so you do release the deprecation in a minor?

By "deprecation" I mean decoration with [Obsolete].

[blairconrad]

Some minor release will have the deprecation.

Oh! I get it. You're reading

(2) issue a new minor release with the deprecation in place. Before you completely remove the functionality in a new major release there should be at least one minor release that contains the deprecation so that users can smoothly transition to the new API.

as

(2) immediately issue a new minor release with the deprecation in place. Before you completely remove the functionality in a new major release there should be at least one minor release that contains the deprecation and that it should occur between the time that you decide to deprecate a feature and the next major release so that users can smoothly transition to the new API.

That's why we're not quite lining up. I suppose this is an interpretation, but I don't think having the Obsolete first show up in a new major release precludes users from smoothly transitioning to the new API. If they aren't willing to pick up a new major API (with potentially (other) breaking changes) they won't be affected by the deprecated/obsolete feature, so there's no real concern. One could say that the transition isn't as smooth. One could also say that it's super smooth because change-averse users aren't being jostled by the deprecation at all…

[adamralph]

This is getting confusing. 😆

Would it be correct to say that your definition of "deprecation" is:

a decision to deprecate something, documenting that, and adding a replacement if appropriate

?

And therefore you are saying that you do indeed "release" a deprecation in a minor?

My definition of "deprecation" is

adding [Obsolete]

I believe you are only doing that in a major, correct?

[thomaslevesque]

I believe you are only doing that in a major, correct?

Yes
And then we remove the feature completely in the next major.

[blairconrad]

I see @thomaslevesque got in before I could finish this comment. I'ma finish anyhow!

---

A concrete example may make it more clear.

Let's take the change in call repetition syntax that was (somewhat) recently introduced. It was possible to add the new API alongside the old one until we removed the old one.

• We came up with the idea to make the change.
• in 4.4.0, we provided the new API, and documented it. We did not obsolete the old one. Fun fact: Mertsch asked why not!
• in 5.0.0 we deprecated the old API
• in 6.0.0 we removed the old API

As you say, the [Obsolete] was only added in a major. It's true that adding the [Obsolete(false)] in the 4.4.0 release might give clients earlier, more visible, notice that they should adopt the new way of working, but there was no real need to give users a new spate of warnings with the minor release. Waiting until the major was fine.

Again, not fighting a switch to another model (for example the one NServiceBus uses), just saying that I don't the current one is so horrible.

[adamralph]

@blairconrad thanks for the clarification.

It is these statements that confused me:

I believe that in practice there's always a new minor before the major.

Some minor release will have the deprecation.

Anyhow, it all goes back to the same question: is adding [Obsolete("...", false)] breaking or non-breaking?

IMO it's non-breaking, and IYO (up until now at least) it's breaking. That's fine. I wanted to provide you with another POV for consideration, but ultimately the decision is, of course, yours.

[blairconrad]

Sure thing, @adamralph. And the NSubstitute way is interesting. But I do think there's a question beyond whether Obsolete is breaking or non-breaking, though. The question I keep coming back to is "is the benefit of adding [Obsolete] to the old endpoint in a minor release large enough to justify potentially disrupting the workflow of clients who hadn't planned for such a change?". Whether or not we think that a non-breaking release may introduce new warnings, maybe they don't.

In particular, I'm thinking of a scenario where a client using version 4.3.0, 4.4.0 deprecates an endpoint via [Obsolete], and 4.5.0 adds a cool new feature that the client wants (or 4.4.1 includes a bug fix that they want). Then when they upgrade, all of a sudden they're faced with the need to amend dozens or hundreds of call sites, or to mess with their compiler settings to suppress a warning.

[thomaslevesque]

And the NSubstitute way is interesting.

I assume you meant NServiceBus. I don't think @adamralph left FakeItEasy to go working on NSubstitute 😄

[afakebot]

This change has been released as part of FakeItEasy 7.0.0-beta.1.

[blairconrad]

This change has also been released as part of FakeItEasy 7.0.0.

[rmiller333]

I see in the linked issue that clearing the configuration supposedly removed its strictness. My use case is an integration test using a Test WebApplicationFactory in dotnet. I get an httpclient from that and for test performance reasons we prefer to do that once per test class, not per test. The client caches its server so without an ability to change fakes we're a bit out of luck. I can override it, but if I don't carefully make each fake a "once" call, the config will bleed from test to test. Any better ways around this?

[thomaslevesque]

I see in the linked issue that clearing the configuration supposedly removed its strictness

Not supposedly; it definitely did (which was not intended, of course).

My use case is an integration test using a Test WebApplicationFactory in dotnet. I get an httpclient from that and for test performance reasons we prefer to do that once per test class, not per test. The client caches its server so without an ability to change fakes we're a bit out of luck.

I feel like I'm missing something here. The HttpClient provided by WebApplicationFactory is a real HttpClient, not a fake. At which point are fakes involved? Could you perhaps show some code so that we can better understand your problem?

[rmiller333]

Our test factory subclass allows setting certain internal objects' Dependency Injection. Essentially it calls this ReplaceService method on the mocks. Then the client gets created and future calls to ReplaceService don't affect the cached server so we're stuck with the service that gets assigned when the client is first created (and if tests run out of order it can cause chaos).

private void ReplaceService<TAbstraction, TImplementation>(
            IServiceCollection serviceCollection,
            TImplementation obj)
            where TAbstraction : class
            where TImplementation : class, TAbstraction
        {
            this.container = null;

            var serviceDescriptor = serviceCollection.FirstOrDefault(descriptor => descriptor.ServiceType == typeof(TAbstraction));
...
            serviceCollection.Remove(serviceDescriptor);

            switch (serviceDescriptor.Lifetime)
            {
                case ServiceLifetime.Scoped:
                    serviceCollection.AddScoped<TAbstraction, TImplementation>(p => obj);
                    break;

Solutions (both crummy):
Do not reuse the factory in multiple tests (slower)
Be VERY careful in configuring the mocks to only run n times, and verify they only those n times so they configs don't bleed over into other tests (error prone)

[thomaslevesque]

Ah, I see. Indeed, that's a scenario where ClearConfiguration() could have been useful. And the workarounds are definitely not great...
Could you perhaps register the fakes services as scoped? This way you'd have a new one for every test. Of course it's not that simple, since singleton services can't depend on scoped services, so it's not a good general solution...

@blairconrad any idea on how to solve this?

[thomaslevesque]

Or maybe you could do something like this:

private IMyService _myFakeService;
...

services.AddTransient<IMyService>(serviceProvider => _myFakeService);

The service is registered as transient, so the delegate is evaluated every time, but will always return _myFakeService, which you can replace during tests. Also, it can be injected into singletons, since it's not scoped.

[blairconrad]

Hey, @rmiller333. Sorry you're having problems. @thomaslevesque, I was going to suggest some variation on "provide a service that proxies the Fake and just replace the Fake underneath". This is no different than your suggestion, only you have code!

I suppose another option might be to instead of clearing configuration, configure the Fake to DoNothing on all incoming calls. It's not the same as clearing out the rules list, but it should have a similar effect.

[thomaslevesque]

I suppose another option might be to instead of clearing configuration, configure the Fake to DoNothing on all incoming calls. It's not the same as clearing out the rules list, but it should have a similar effect.

It's not the same in the sense that it doesn't remove existing rules, but it still overrides them, so I think it produces exactly the same effect.

So basically, where you were doing Fake.ClearConfiguration(fake), you can do A.CallTo(fake).DoesNothing(). That's probably the easiest workaround. Note that it has the same problem as ClearConfiguration: it removes the fake's strictness.