Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using multiple DataRowsAttributes makes the test non CLS-compliant #1740

Closed
martinsuchan opened this issue Sep 26, 2023 · 9 comments · Fixed by #1878
Closed

Using multiple DataRowsAttributes makes the test non CLS-compliant #1740

martinsuchan opened this issue Sep 26, 2023 · 9 comments · Fixed by #1878

Comments

@martinsuchan
Copy link

martinsuchan commented Sep 26, 2023

The change #1503 and #1646 introduced regression where test methods with multiple DataRowsAttributes are now no longer CLS-compliant.

I discovered this issue in our codebase where we use shared AssemblyInfo for all projects including tests with
[assembly: CLSCompliant(true)]
Together with "Treat warnings as errors" that we use updating MSTest rom 3.0.1 to 3.1.1 effectively breaks our build.
I know this is probably very edge case scenario, CLS complinace in test projects has not much meaning, but in our case I need to manually add CLSCompliant(false) to about dozen test classes to fix it.

Expected behavior

No breaking change when updating MSTest from 3.0.1 to 3.1.1

Actual behavior

Cannot build solution without manually changing dozen+ of our test files.

Simple repro:
image

AB#1925774

@martinsuchan
Copy link
Author

Note, can be reproduced in VS 2022 17.7.4 in net472 and net6.0 test project.

@Evangelink
Copy link
Member

Hi @martinsuchan! Thanks for the bug report. I definitely need to add a test case for CLS compliant as you are not the first one reporting an issue with it. We will work on a fix asap :)

@Evangelink
Copy link
Member

Hi @martinsuchan!

Sorry for the delay in handling this issue. I am working on a fix and would like a small confirmation that all cases raising an issue are cases where you have only one item declared?

@martinsuchan
Copy link
Author

martinsuchan commented Dec 5, 2023

Hi, just a note, you should probably add more tests to the ClsTests class, including with zero, one or more DataRows.
We are using multiple DataRows of items in our tests.
Also the problem I reported was not with tests not passing, but with build warnings. I don't see any place checking if there are no build warning about CLS compliance?
8c9530e

@Evangelink
Copy link
Member

Reopening to explore the testing part. Thanks @martinsuchan artin

@Evangelink
Copy link
Member

@martinsuchan I have done a couple of manual tests and it seems that there is no strong need for a more complex test. Because the test assets are built, if we regress, we will start to have some warnings triggered and because we have warnings as errors in CI, this would ensure we are aware of the change.

@martinsuchan
Copy link
Author

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)]

@Evangelink
Copy link
Member

I can only reproduce the issue with [DataRow(null)], I am investigating what I can do.

@martinsuchan
Copy link
Author

martinsuchan commented Jun 13, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants