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

Add Assert.IsInstanceOfType<T> #1241

Merged
merged 15 commits into from Sep 15, 2022
Merged

Add Assert.IsInstanceOfType<T> #1241

merged 15 commits into from Sep 15, 2022

Conversation

engyebrahim
Copy link
Member

@engyebrahim engyebrahim commented Sep 12, 2022

Fixes: #413

Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

Please rebase on main.

# Conflicts:
#	src/TestFramework/MSTest.Core/Assertions/Assert.IsInstanceOfType.cs
#	src/TestFramework/MSTest.Core/Assertions/Assert.cs
#	test/UnitTests/MSTest.Core.Unit.Tests/Assertions/AssertTests.AreEqualTests.cs
#	test/UnitTests/MSTest.Core.Unit.Tests/Assertions/AssertTests.IsInstanceOfTypeTests.cs
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

Let's discuss offline a couple of things:

  • Shall we deprecate the old API in favor of this one?
  • Shall we use the same pattern as for other APIs (i.e. not using CallerArgumentExpressionAttribute) and do the update globally in v3? Or shall we add new principle directly now knowing we would have 2 kinds of API (inconsistent behavior for users).

# Conflicts:
#	src/TestFramework/MSTest.Core/Assertions/Assert.IsInstanceOfType.cs
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

Globally LGTM! Could you please fix the 2 broken tests.

@Evangelink
Copy link
Member

Please update the description to mention the ticket this PR relates to.

Evangelink
Evangelink previously approved these changes Sep 15, 2022
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

One final nit.

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.

Add Assert.IsType<T>(object obj)
3 participants