-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add more System.Xaml tests #8203
Conversation
30733d2
to
878511f
Compare
There's a faulty debug assertion in
|
6f117bb
to
ef3fec1
Compare
Awesome |
ef3fec1
to
d2a7915
Compare
...DotNet.Wpf/tests/UnitTests/System.Xaml.Tests/System/Xaml/Permissions/XamlAccessLevelTests.cs
Outdated
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Some tests failing due to the change to return |
409cd6a
to
4cdbd16
Compare
@hughbe the |
13a2c4b
to
000ec2d
Compare
000ec2d
to
5147840
Compare
src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Xaml/InfosetObjects/XamlNodes.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Xaml/Schema/TypeReflector.cs
Outdated
Show resolved
Hide resolved
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.
LGTM.
@hughbe
Thanks for your contribution!
Are you planning to add more test?
If not, I can continue to merge it.
I'm not planning to add any more tests :) Thanks for your review |
Disclaimer: I haven't gone through the PR. However, I am worried about the asserts being removed, presumably they were added for a reason and express an expectation of the code that follows. Either the tests should be fixed to not hit them or if we think they are unreasonable, the code needs to be checked that it can deal with the conditions being broken. |
If there are situations where Debug asserts can be hit by unit tests, I've only ever seen us removing them (and fixing any downstream bugs, e.g., NREs etc.). |
@hughbe It seems like the only remaining point of discussion in this PR is about the removal of asserts, would it be easy to extract the tests that requires the removal of asserts and open a separate PR with them ? If so, I think it would be a good idea to do so to be able to merge this PR as soon as possible and the changes in this PR would be less controversial since there would be no non-test changes. |
5147840
to
de9940b
Compare
@hughbe, I have looked at the PR and the changes LGTM, however there are two style errors which are causing the issue here : IDE0059 and IDE0073. Can you fix these issues in the PR ? |
81a61bb
to
afcc0dd
Compare
Apologies for the delay. I have just fixed all the IDE errors and failing tests |
Thanks @hughbe, I was just getting started with using the NoAssertContext mentioned in the other pull request. Thanks for fixing this though. |
Thanks a lot @hughbe for the PR, and apologies for the delays from our side. I will move on to reviewing the other PR. |
Thank you! Really glad this was merged in :) |
Had these lying around from 5 years ago. Can now send in after #138
Microsoft Reviewers: Open in CodeFlow