Skip to content

Issue4416 #4417

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

Merged
merged 49 commits into from
Jul 20, 2023
Merged

Issue4416 #4417

merged 49 commits into from
Jul 20, 2023

Conversation

OsirisTerje
Copy link
Member

@OsirisTerje OsirisTerje commented Jul 11, 2023

Resolving #4416

Copy link
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have fixed some of the items mentioned:
Enabled warnings for formatting violations and fixed code.
Added Assert.cs class
Enabled Assert.Conditions.cs. Note that there no longer seem to be unit test for these (probably all converted to Assert.That).

Let me know if you want me to do some more work for you.

@OsirisTerje OsirisTerje mentioned this pull request Jul 15, 2023
@OsirisTerje OsirisTerje changed the title WIP: Issue4416 Issue4416 Jul 15, 2023
@OsirisTerje
Copy link
Member Author

OsirisTerje commented Jul 18, 2023

@manfred-brands

@OsirisTerje I think our tests are to verbose in general. With all the output it is hard to find if/where there is an error.
When running tests in VS yesterday, I noticed lots of output from Assume expected True but was False

Some of these comes from platform dependent tests. I believe we could use the Platform attribute instead to get rid of these.
Some comes from Theory based tests, but I don't understand why the tests are made that way. It is just confusing.

I notice that the tests are still visible in the Test Explorer, but I thought that #3866 should remove them?

[UPDATE]
The results coming back from these tests are either Skipped (Platform stuff and Explicit) or Inconclusive (Theory and other Assumes). It's possibly best to let these pass through to the output as they do now, but they could be optionally filtered in the adapter. That's an easy place to do so, and would fix both dotnet test and Visual Studio.
In the VS Test Explorer you can already now filter these off there, by selecting only Green and Red tests, but it would be nice to get them off the CI builds, as you say - they are too verbose and hard to interpret.
It would also help to actually see explicitly that result comes from an Assume. Now it can be inferred by seeing that they do give a result and at the same time is Inconclusive, --- but Test Explorer only says it is Not Run.
When we get the CAE, this will be more visible.

@OsirisTerje OsirisTerje merged commit ca52593 into master Jul 20, 2023
@OsirisTerje OsirisTerje deleted the Issue4416 branch July 20, 2023 17:53
@mikkelbu
Copy link
Member

Regarding the inconclusive tests, then it has been an issue for a long time - see e.g. #1001

@OsirisTerje
Copy link
Member Author

Awesome, thanks @mikkelbu ! I was not aware of that one. I think we can actually handle this in the adapter, and and then turn those off in a .runsettings.

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

Successfully merging this pull request may close these issues.

3 participants