search

microsoft / testfx

MSTest framework and adapter
MIT License
732 stars 254 forks source link

DataRow regression in support of CLS compliant #3111

Open Evangelink opened 3 months ago

Evangelink commented 3 months ago

Hi, we just discovered when trying to upgrade to version 3.4.3 that this problem is actually not fixed. We have a test method with following Data rows, and the build fails on non-CLS compliant warning/error. Please add this variant to your CLS test suite, you should be able to reproduce the problem, thanks. @Evangelink 

[DataRow("")]
[DataRow(null)]

Originally posted by @martinsuchan in https://github.com/microsoft/testfx/issues/1740#issuecomment-2164992993

Similarly this is now reported as non-CLS compliant:

[TestMethod]
[DataRow("some string", 1)]
public void StringIntDataRow(string s, int i)

Note it is possible that this non-CLS compliant behavior is the cause, or it's related to #3071 - if non-compliant tests are compiled to native code in UWP projects it might cause some troubles?

Originally posted by @martinsuchan in https://github.com/microsoft/testfx/issues/1740#issuecomment-2166884193

Evangelink commented 3 months ago

Hi @martinsuchan,

Thanks for the feedback and sorry to see there is still something broken. I have been investigating the issue and I have discussed with Roslyn team because it seems we are on some dead-end on our side and sadly it seems that CLS compliant is not really supported. I'll discuss with the team to see what we could do because we will need to either keep the current breaking state (and update the notes to mention it) and possibly introduced a ClsDataRowAttribute or we will need to break some other supported scenario.

martinsuchan commented 3 months ago

Hi, the thing that I don't understand - there were no CLS errors with DataRows in version 3.0, but then it started in version 3.1. So I'm wondering what was the reason that caused this and if it's possible to go back to the behavior or implementation we had before?

Evangelink commented 3 months ago

Hi,

I did a bug fix in 3.0.3 (if I recall correctly) for the available ctors because some were broken as part of the refactoring/breaking changes in 3.0.0 because I broken some main scenarios in the process. If I rollback, this will be fixing your case but will be breaking some others that seem to be more mainstream.

I have been discussing with compiler team and they told me that there are many bugs on compiler side related to CLS that they don't plan to be fixing.

I am looking at other solutions I could put in place but so far, the only one that seems to be working is to introduce a new attribute. Would you be willing to make changes on your code?

martinsuchan commented 3 months ago

I think that for us it will be easiest to stay on the version 3.0 for now. Introducing new ClsDataRow feels like an overkill. One question remains if this CLS issue is related to #3071 - if it's not possible to use DataRows for standard UWP tests at all?

Evangelink commented 3 months ago

I will be investigating the UWP part soon and post back the results.

Again I am sorry for the broken state I have put you in.

Evangelink commented 2 months ago

I forgot to post back here that the problem on UWP is not linked to the CLS change.