-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 WindowsBase tests #8215
Add WindowsBase tests #8215
Conversation
90229fe
to
f7622fc
Compare
src/Microsoft.DotNet.Wpf/tests/UnitTests/WindowsBase.Tests/WindowsBase.Tests.csproj
Outdated
Show resolved
Hide resolved
c4b0418
to
60cdc7c
Compare
60cdc7c
to
9480458
Compare
69a8dcb
to
2e394c6
Compare
Hey @hughbe, are you still planning on working on this ? If yes, could you rebase on main to include my PR #9471 which should fix the WindowsBase conflict that you saw and other errors that you would have gotten at build and runtime. I pulled your changes locally and ran the tests. Rebased on main and with a couple of other infra changes, the tests were running and the failures seemed specific to the code. Let me know and I'll send you my changes. |
Hi there - yes I am planning on advancing this. I will try to do so this week! |
2e394c6
to
3f5dbfb
Compare
99369e2
to
c7467ff
Compare
Errors on the debug build:
|
Hey @hughbe, could you rebase on main ? My PR #9471 was merged in main today and it should fix the errors you are seeing. I pulled your branch locally and I was able to run the tests. There's a couple more things that needs fixing in your PR:
Logs for the point 2
|
aa2ddea
to
51f454c
Compare
@JeremyKuhne would you like me to rebase with or without the changes to remove the invalid debug asserts that crash the tests? |
@hughbe I don't have a strong opinion either way. I've added the NoAssertContext to the shared test assembly if that will help you any. dotnet/winforms#12621 It should show up in WPF in a couple days. |
/azp run |
Pull request contains merge conflicts. |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@hughbe, I was trying to debug the tests further, and although the tests run without any issues when ran via Test Explorer in Visual Studio, but when we run them using CLI using the following command
the tests that are meant to test classes derived from WeakEventManager, like CollectionChangedEventManagerTest, CurrentChangedEventManagerTests, WeakEventManagerTTests are crashing with unhandled exceptions with the following error message
or an invalid cast exception to some other type depending on the tests. On looking further found that the NamedObject has the I am looking more into this, but would like to know if you have some more inputs on this. UPDATE : I am guessing this has something to do when we are passing
UPDATE 2 : Here is a commit disabling a few tests which fixes the InvalidCastException : e9429c4 |
Hi there. Sorry for the terrible feedback loop from me on this! I’ve been moving home and haven’t been able to dedicate the sort of time it deserves. my initial theory is that there could be a bug in WPF where we’re forcing a cast to some sort of internal class that end users can’t instantiate (w/o reflection) a fix could be to add |
I am not sure about what is the right fix for this right now. However at this moment, I think it would be best to disable the few tests and take them up as a separate issue. I tried disabling a couple of them in this commit : e9429c4 and did not ran into the exception.
That's okay, nothing to worry about. We haven't been good at it either. 😅 |
@hughbe, what are your thoughts on disabling the 5-6 tests ? If you are okay with it, I can push a commit disabling those tests and move the PR ahead. |
Yes, let's go ahead with that thanks |
@dipeshmsft whole DRM doesn't work afaik so u might as well disable the tests to it. |
@h3xds1nz DRM ? |
@dipeshmsft The SecureEnvironment thing with RightsManagementTransform stuff. Basically stuff that requires PresentationHost |
The tests are still working for that area so I am not removing other tests. However there is a Dispatcher test failing which may need disabling for now. |
@dipeshmsft Ye I see, as for the failing test it needs to be |
@h3xds1nz, but it worked this time. It is kind of flaky. And everytime just the first test fails in the DispatcherTests class. @hughbe, any comments on this ? I am thinking of keeping it this way, if the failure happens for multiple PRs then I will disable the |
I reproduced it locally multiple times; I'm currently dealing with a race condition in System.Xaml tests from previous batch; we should make sure the tests are stable as well. |
In that case, I should update all the tests in DispatcherTests to |
Yeah, in the DispatcherTests case we should. I went through them briefly and don't think there's one that shouldn't. |
Thank you! |
Wrote these a long long while ago, sending in for CI
Microsoft Reviewers: Open in CodeFlow