Consider being able to set new TestNodeStateProperty without recreating the TestNode

[thomhurst]

As per this comment:

          Yes that is how it is right now.

Originally posted by @nohwnd in https://github.com/microsoft/testfx/issues/2551#issuecomment-2078978919

You should create a new TestNode/PropertyBag for each new test state, and these new nodes should still contain the well known properties such as TestMethodIdentifierProperty and TestFileLocationProperty.

So a test could go: Discovered > InProgress > Passed/Skipped/Failed

Because the property bag doesn't have any public methods for removing state/properties, I cannot re-use a test node that's been sent in a previous state.

So I have to reconstruct my test node again, and re-populate all the data for the well known properties, etc. And the more tests I have, the more I have to do this obviously.

Discovered(New Node w/ DiscoveredState) > InProgress(New Node w/ InProgressState) > Passed/Skipped/Failed(New Node w/ Passed/Skipped/FailedState)

This means a lot more allocations, and more work to new up all the required metadata again and again.

Would it not make sense for a new state property to simply replace the old state property? Currently that throws an exception.

Discovered(New Node w/ DiscoveredState) > InProgress(Same Node w/ added/replaced InProgressState) > Passed/Skipped/Failed(Same Node w/ added/replaced Passed/Skipped/FailedState + extras such as TimingInfo etc)

Keen on your thoughts. Feels like it'd be more performant?

[MarcoRossignoli]

Because the property bag doesn't have any public methods for removing state/properties, I cannot re-use a test node that's been sent in a previous state.

It's by design, when you push a node update inside the message bus the IData is sent to all the subscribed extensions. We don't know what the extensions will do with the IData, they could save in their local. So it's correct re-create a new instance of the node with same identity(uid) and new state and push it again.

We were thinking to "lock" everything once pushed (IImmutableData: IData with a method MakeImmutable()) , like also the add would fail to make the object "runtime immutable" (cc: @Evangelink)

We discussed a lot about immutability and we had to trade it off with extensibility and future new feature keeping the contract more interface(light with less embedded semantics as possible) based, making desired semantic a bit more complex to achieve.

[thomhurst]

I think making other parts immutable (name, id, location, etc) makes sense, but maybe not the test state property. Treat that as a special case where a new state just simply replaces the old one. It's treated specially ATM Anyway, but it just throws an exception. Recreating the test node with all the same data again apart from the different state just seems unnecessarily expensive in terms of compute?

[MarcoRossignoli]

Treat that as a special case where a new state just simply replaces the old one.

It's related to correctness, suppose the you push your test node with some property in the bag plus the state of it. Two IDataConsumer will save it in a local list and at some random point the adapter changes or remove the state of it during a read from the consumers(everything is async and parallel). Or again the consumer changes the property for a bug , a task that should be done by the adapter.

How do we protect the "state" from bugs like that? How complex is to find out what's wrong in case of it? When a platform is extensible we cannot take any "strong" decision on the code that will implement the extension. So we need to protect all other part from possible wrong external code.

Recreating the test node with all the same data again apart from the different state just seems unnecessarily expensive in terms of compute?

If you have thousands of tests will be thousands x 2 (in progress, final result) I'm pretty sure that a test suite creates a lot more objects than that. Also the bag is optimized and it's not using arrays but linked list and an empty one , we don't expect a lot values in it. We provide also some cached value for standard result like https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Platform/Messages/TestNodeProperties.cs#L56

We will also add a new option called --interactive that can be used by all extensions to express the fact that a user is in front of the screen "interacting" so extensions can decide how many info to push. For instance if we're not in interactive mode(CI) you don't need the in-progress nor information about test code line etc...so that would improve the performance "based" on the context.

[thomhurst]

I see what you mean.

Could we not have two objects maybe?

When we're within the test framework code itself we can hold a TestNode + PropertyBag and they are mutable. And then data producers and consumers take an ImmutableTestNode + ImmutablePropertyBag. There could maybe even be an implicit conversion operator so you can pass in a standard test node when publishing and the consumers are then passed an immutable version.

It'd sort out the problem of consumers manipulating data.

Although now thinking of this, it's still allocating more data. Maybe they could be structs that just wrap around the original object and throw exceptions on any Add/Edit methods.

[thomhurst]

Also part of your argument is not modifying data, but couldn't one consumer currently start adding properties and affect another consumer?

[MarcoRossignoli]

Although now thinking of this, it's still allocating more data. Maybe they could be structs that just wrap around the original object and throw exceptions on any Add/Edit methods.

"Copy" the values is at the moment the easiest and safest way to avoid hard concurrent issues, the direction we decided to take is to be "immutable" as possible, with some tradeoff for usability.

Also part of your argument is not modifying data, but couldn't one consumer currently start adding properties and affect another consumer?

That's true, reason why we're thinking to lock the bag once pushed and throw at runtime if someone tries to mutate the list of element, or introduce a PropertyBagBuilder that with an ToImmutable() that will set an ImmutablePropertyBag or make it IReadOnlyList.

[MarcoRossignoli]

@Evangelink I think that we could/should abandon the idea of "easy syntax to create objects"(init etc...) in favour of ctor and full runtime immutability. We did it to be good as possible with different languages, but looks to me we cannot do if partially, we don't own c# or f# and we cannot introduce sugar. I would trade that with complete immutability.

[thomhurst]

Here's the idea I had for immutability: https://github.com/thomhurst/testfx/pull/2/files

I make the assumption though that every push to the message bus should have a test state associated with it.

So you have to call ToImmutableTestNode(...) on your builder classes and pass in the current state.

The builder classes you can keep and re-use to populate new immutable TestNode + PropertyBag instances. This makes it a lot easier to build new test node objects without having to essentially rebuild them from scratch.

The PR doesn't compile btw, as it'd be a massive change and affect a lot of tests etc. But just wanted to give an idea.

[MarcoRossignoli]

Moved to the first breaking change https://github.com/microsoft/testfx/issues/3473 thanks @thomhurst