Description, WorkItem, CssIteration, CssProjectStructure should *NOT* be treated as traits.

[singhsarab]

Attributes like Description should not inherit from TestPropertyAttribute, as eventually TestPropertyAttributes are treated as traits.

Fix for #478

Docs: microsoft/testfx-docs#54

[jayaranigarg]

@AbhitejJohn : Any specific reason why these attributes were inherited from TestPropertyAttribute?

[AbhitejJohn]

@jayaranigarg : This seems to explain it. you might want to revise that doc if this changes any of those behaviors for the default attributes.
/cc : @jfleisher .

[jayaranigarg]

@singhsarab : Can you please update that doc as well as part of this change?

[singhsarab]

Associated changes in the docs. microsoft/testfx-docs#54

[dotMorten]

These appears to me as breaking changes. Was that intended?

[singhsarab]

@dotMorten Can you please share the concerns you have around this change ?

[dotMorten]

While changing a class to inherit from a subclass of the original class isn't a breaking change, doing the opposite as what is being done here is.

Scenario:
Library 1 references Nuget1 v1.3.2
Library 2 references Nuget v1.3.3 with the inheritance change
App is referencing both, and thus needs to reference 1.3.3, but hits code in Library 1 that in turn hits this changed class. Due to a break in the binary compatibility, Library 1 is no longer working as it can't find the members it expects in the location it expected. You'd have to contact the author of Library 1 and ask them to upgrade to 1.3.3 and recompile, before you can continue t use both libraries.

Breaking changes should only be done in major versions, ie you'd have to waif for v2.0.0

[jayaranigarg]

@dotMorten : We don't see anything breaking in this change as of now. We certainly will validate this scenario before releasing a higher version of adapter and will revert/make appropriate changes if we see any issues.

[dotMorten]

@jayaranigarg You probably aren't seeing anything because you recompiled everything and didn't worry about binary compatibility. See the guidelines here:
https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/breaking-change-rules.md
Specifically this one:

Removing one or more base classes for a type, including changing struct to class and vice versa

[dotMorten]

...also any class accepting the base type can no longer pass instances of the derived attribute, so it's also a breaking change at the source code level.
Example. This would now break:

DescriptionAttribute da = new DescriptionAttribute();
var attribute = (TestPropertyAttribute)da; //Invalid cast
var key = da.Key; //No longer exist
var value = da.Value; //No longer exist

[singhsarab]

@dotMoren I understand the from Oops perspective, it seems like a breaking change.
But thinking from the perspective of what this code is used for, this change seemed harmless.

• These are sealed classes, so cannot be extended.
• Users use these attributes only to decorate their unit test.
• I cannot think of the scenario where the user will actually try to create an object of the attribute and cast. Maybe we are missing it, can you please share some real life scenario where this is relevant.

I am actually fine bumping up the version for the adapter if we really think it breaks the users.
Also, this change has already been rolled out the pre-release version 1.4.0-beta and we haven't heard back on it.

[ahmedalejo]

• Users use these attributes only to decorate their unit test.

i agree with @dotMorten if a class public and has been released, there is no way you can presume that developers only uses the library the way you expected. Especially using reflection for code analysis.

There´s no problem with the change itself, only with the version update.

Changing the class hierarchy of a public class is a breaking change and should be a major version update 2.* and not 1.* but since this is a preview release i´m sure there wouldn´t be a problem fixing that.

[dotMorten]

I cannot think of the scenario where the user will actually try to create an object of the attribute and cast.

That's the thing with an API design: You don't know every possible use out there, so you have to evolve your API very defensively. That's just the harsh (and IMHO fun) nature of building APIs.

But as an example I interrogate the attributes heavily in my own TestMethod to better drive execution and logging. There are also custom test runners (typical in Xamarin etc) that'll use the attributes for running the tests on devices.

Now don't get me wrong: I think the change is a good change - had it been done to begin with. But IMHO that ship has sailed.

[singhsarab]

@ahmedalejo @dotMorten I agree, in future, we will be more careful with the versioning.
We haven't received any reports with respect to this change though, we release 1.4.0 with this change, half a million downloads already.