-
Notifications
You must be signed in to change notification settings - Fork 284
Is subsetof return non subset values of the superset (relates to #662) #6292
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
base: main
Are you sure you want to change the base?
Is subsetof return non subset values of the superset (relates to #662) #6292
Conversation
Sorry for the delay on reviewing this @AtolagbeMuiz. We will try to get to it soon. Meanwhile, could you read the Contributor License Agreement (CLA), and if you agree with it, accept it, per the instructions in #6292 (comment) |
Hi @Youssef1313 , I have read it already but I don’t know how I can agree to the License Agreement. I was expecting to see a button to press “Agree”. Kindly, explain how I can do this. Thanks. |
You need to add a comment in the PR with text |
@microsoft-github-policy-service agree |
@Youssef1313 please have you got idea while this build failed?? happened after i updated my branch to sync with main |
Sometimes it's just flakiness. Usually we can restart the failing job and it will pass. We are working on improving the stability of our tests to avoid flakiness. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, LGTM! Only few comments to address.
…o make the variable localized
…://github.com/AtolagbeMuiz/testfx into IsSubsetOf_ReturnNonSubsetValuesOfTheSuperset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AtolagbeMuiz.
One final thing to consider, it would be very great if you can add some unit tests to assert the message
…sage when a collection isn't a subset of the superset
This has now been done, and the PR updated. unit tests were written to assert the message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, found few things that I would suggest revisiting. Please let me know if you have any concerns, or need to discuss it further.
src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.cs.xlf
Outdated
Show resolved
Hide resolved
test/UnitTests/TestFramework.UnitTests/Assertions/CollectionAssertTests.cs
Show resolved
Hide resolved
…s.cs.xlf Co-authored-by: Jakub Jareš <[email protected]>
…alized string message and also SubSet property name
This PR fixes IsSubset should return a better default AssertMessage #662
This implementation allows The CollectionAssert.IsSubsetOf default message to have more information regarding the message being returned. This message returns value(s) that is/are not a subset of a given superset.
Current Behaviour
Currently, The Message only says
CollectionAssert.IsSubsetOf failed.
Expected Behaviour
With this Implementation, the message becomes more explanatory by giving more Information on what elements are missing in the collection. The message that will be returned is
CollectionAssert.IsSubsetOf: values {"test1", "test2"} is/are not a subset of the superset