Register property attributes during deserialization #1644
Conversation
|
@singhsarab @cltshivash - could we get this reviewed and merged into d15.8stg branch for preview 4 release? |
|
@dotnet-bot retest Windows_NT / Release Build please |
| @@ -71,7 +71,9 @@ public override object ReadJson(JsonReader reader, Type objectType, object exist | |||
| testCase.LineNumber = int.Parse(propertyData); break; | |||
| default: | |||
| // No need to register member properties as they get registered as part of TestCaseProperties class. | |||
| TestProperty.Register(testProperty.Id, testProperty.Label, testProperty.GetValueType(), typeof(TestObject)); | |||
| // Use a previously registered property if one exists. | |||
| testProperty = TestProperty.Find(testProperty.Id) ?? | |||
There was a problem hiding this comment.
Register already does a find internally, & then adds the property if not present, do we need do an explicit find?
There was a problem hiding this comment.
I had taken a look and it seemed like it would add the owner to a hashset if the supplied owner is different. I was not sure that is necessary since the owner doesn’t seem to be used by anything...
It also seemed cleaner and more explicit to do a Find to avoid confusion (such as what if the property is already registered with different attributes in the process?)
I can certainly omit the call to Find if you disagree.
There was a problem hiding this comment.
I have gone ahead and removed the Find.
| @@ -100,7 +100,9 @@ public override object ReadJson(JsonReader reader, Type objectType, object exist | |||
| testResult.ErrorStackTrace = propertyData; break; | |||
| default: | |||
| // No need to register member properties as they get registered as part of TestResultProperties class. | |||
| TestProperty.Register(testProperty.Id, testProperty.Label, testProperty.GetValueType(), typeof(TestObject)); | |||
| // Use a previously registered property if one exists. | |||
| testProperty = TestProperty.Find(testProperty.Id) ?? | |||
Description
When registering properties as part of deserialization, we are currently registering with the wrong attributes. This means, for example, that the registered
MSTestDiscoverer.TestCategoryproperty in devenv.exe would end up without theTestPropertyAttributes.Traitattribute.The effects of this are unobservable for tests discovered via container/reflection based discovery (CBD) (since
testcase.SetPropertyValues()is called with the deserializedtestProperyobjects directly as opposed to the property that was registered). However, this leads to problems for tests reported via live unit testing (LUT) and source based discovery (SBD). Both LUT and SBD need to applyMSTestDiscoverer.TestCategoryproperty on tests and they callTestProperty.Find()to find existing an property with that id. If CBD has already happened LUT and SBD will find the property registered by the below code with missingTestPropertyAttributes.Traitattribute. Consequently new tests reported via LUT and SBD end up under theNo Traitsgroup in the test window if user has grouped their tests by traits. Changes in traits for existing tests updated via LUT are also not reflected in this view. Worse still, the problem only repros if CBD has happened before SBD or LUT (which can be somewhat racy because both LUT and CBD can be triggered parallelly after solution load).The fix ensures that we when we register properties we also apply the correct attributes so that any other consumers who call TestProperty.Find() within the same process (or rather AppDomain) will find the correct property. I also made another small change to use a previously registered property if one exists instead of trying to register the same one repeatedly.
Related issue
I haven't logged an issue on this repo for this although this was discovered as part of investigating this problem report on the developer community: https://developercommunity.visualstudio.com/content/problem/263657/live-unit-testing-in-vs-preview-shows-no-traits.html. Please let me know if I should also open an issue in this repo.