TestContext.CurrentTest exposes too much internal info

[CharliePoole]

The following problems should be fixed... and the code examined for any others...

1. The Properties property exposes IPropertyBag, which is intended as an extensibility interface, not one for users. Among other things, it allows setting properties, which is not a good idea and potentially confusing since they would not be persisted. The internal PropertyBag should be wrapped by an adapter object, just as we wrap the test and the result.
2. The Properties indexer returns an IList. We should determine if this can be made type-safe. If it cannot be done at the present time (IIRC, we have both string and int properties at a minimum) then this should be spun out as a separate future issue.
3. The IList returned by the indexer gives too much access. An IEnumerable would be better. Alternatively, another layer of adapter with enumeration and Count available would do the job.

Hard to know how to prioritize this. I'm marking it Low for now, but if we see other issues arising that relate to this problem we should increase it.

[jnm2]

re: #2665 (comment)

While the question warrants some discussion, I don't think it needs a loglot. It's an error and the breaking change will be a failure to compile. That's a good thing since changing the property bag doesn't currently have any effect. For example, if you set the MaxTime property, you don't suddenly get MaxTime behavior because that was all decided before the test ever ran. IOW, it's a bug to fix.

I'm not concerned for the sake of expecting MaxTime or other behaviors to be retroactively triggered. I'm concerned that someone will have been "fixing" values or adding their own custom names to the bag. We've seen this with test parameters if I remember right. Is that clearly an error?

[CharliePoole]

See correction to my comment above: s/log/lot/

I'm not sure what you mean WRT test parameters. For properties, the current code allows access right into the property bag associated with the test object. It's possible that adding or changing a property would be reflected in the XML output. It's equally likely in my mind that you could crash the test run by changing the properties of a test in the middle of its execution. It would take a lot of experimentation to figure out the limits where this could be done safely and I don't think it's worth the effort because exposing the properties to change was an error in the first place. We've discussed this in several issues in the past, but I'm damned if I can find one of them now that I need it! We also discussed creation of result properties, which could be modified or created by the test but have never acted on it.

Bottom line, the possibility of changing properties was an error I introduced in the first place by simply pointing to the real property bag rather than copying it or providing a read-only interface. Even though it's possible that some user has followed up on my error by changing the properties, I feel no compunction in breaking their code because there is no documentation that suggests they should feel free to do that. I know, we have made exceptions to this in the past, particularly when the bad code came to be relied on by a third-party application with lots of users. I don't think this is such a case, however.

This is about the amount of discussion I thought we needed. Is it enough for everyone else?

[jnm2]

The things to discuss in my mind are:

• What about the possibility of keeping it IPropertyBag but providing an internal adapter implementation which throws for mutating methods? That would keep the code compiling and fail only the misbehaving tests with a diagnostic message. This feels less clean to me than breaking.

• If we don't mind breaking source code, should we add obsoleted methods on the new type to match the mutating methods on IPropertyBag with [Obsolete("Diagnostic message, error: true)] rather than letting people wonder what exactly changed?

[CharliePoole]

If Properties returns a type that includes the methods we want used, then nothing breaks except code that uses methods we don't want used. It shouldn't matter in this case whether the interface is IPropertyBag or IReadOnlyPropertyBag or if there is no interface at all...

TestContext.CurrentContext.Test.Properties.Get("SomeProp"); // keeps working
TestContext.CurrentContext.Test.Properties.Set("SomeProp"); // won't compile

Alternatives to simply leaving out Set are

1. Make it obsolete. This is a misnomer, since it's not something we intentionally ever supported, but it is a way to guarantee a clear error message.
2. Throwing an exception at runtime. This also gives us control over the error message but the user doesn't see it as soon.

I could live with any of the alternatives. My order of preference is (1) Just leave the method out (2) Use Obsolete (3) Throw. The reason I like the first choice best is that we are then done with this and don't have to revisit it again and I'm betting that not many users are affected.

I'd feel differently if I hadn't been involved in so many discussions about how setting the properties will not work or (especially) if our documentation suggested you could effectively change the properties.

[jnm2]

I didn't clarify this in this thread, but unless we go with keeping the type IPropertyBag and having the mutating methods throw at runtime, code that has always used this API properly will break:

var foo = ExtractSpecialProperty(TestContext.CurrentContext.Test.Properties);

// ...

Foo ExtractSpecialProperty(IPropertyBag properties)
{
    // ...
}

[jnm2]

Ignore the (deleted) part about the implicit cast. User-defined conversions to an interface type are not supported in C#.

[CharliePoole]

Let me answer your objection in more general terms. In my view it is always possible to create an example with the aim of breaking something. It is, therefore, always possible for any public change to be called a "breaking change" in the loosest sense. But we call such examples "contrived" for a reason. Most users are not trying to break things and I want to avoid "breaking changes" in a much more restricted meaning of the term: changes that break the code of users who are using the feature as it was designed to be used.

So...

It's absolutely true that if somebody has been passing the value of the Properties property to some method that takes an IPropertyBag as an argument, then the code will not compile. To that I will say two things:

1. This is not the normal use of the Properties property. I seriously doubt that it will affect many users. If you have an example to indicate otherwise, put it out there.

2. For the users it affects, breaking their compile is the best thing we can do for them. It shows the error immediately and in an unambiguous way.

Sorry if I got a bit impatient with your eamples. I was starting from the point of view that it's obvious that the change will break some code. That's the difference between a change that breaks things and a "BreakingChange" as we typically use the term... a change in how the user are expected to use the feature and which we must try to avoid at all costs.

[jnm2]

And that's exactly what I'm hoping for discussion on. How likely do we think it is that legitimate user code is doing anything other than simply dotting through the ITest.Properties property? Is this something you would see yourself writing?

IPropertyBag properties = TestContext.CurrentContext.Test.Properties;
int someValue = int.Parse(properties.Get("SomeParameterName"));

[OmicronPersei]

Well, what can you get functionally out of incorrectly using TestContext.CurrentContext.Test.Properties? Assuming that's disabled, would there be some viable workaround for users affected?

(Honestly, I think there's only so much we can get out of speculating what the NUnit community at large does until we actually go out and ask them in some manner, or wait for complaints if we implement some level of a "breaking" change.)