-
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
Fix flaky tests #10304
Fix flaky tests #10304
Conversation
47039d3
to
e540f17
Compare
e540f17
to
0ea5102
Compare
I've fixed the build errors so this PR is now ready for review. I also had to fix the property _PackagingNativePath because it didn't have the right value when targeting ARM64. |
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.
Just to reiterate, the purpose of the project reference is just to make sure the native dependencies are already built and that is what was causing the flakiness in tests.
For the rest of the changes they are fixing the issue that we did not have the right path of native dependencies for ARM64
@dipeshmsft Yes, and also the changes in IncludeNativeDependencies is to prevent flakiness in the future by making the build fail if the native dependency was not found instead of failing at runtime. This fixes the ARM64 test build (ARM64 tests are not ran in CI currently so that's why the tests weren't failing): This prevents flakiness in the future: This fixes the current flakiness: |
Thanks @ThomasGoulet73, got it. |
@ThomasGoulet73 , I was writing some tests for PresentationFramework.Fluent, but when I do window.Show(), the tests fail with the exception |
@dipeshmsft I was able to reproduce the issue, I'm looking into it. |
Thank you @ThomasGoulet73 for taking a look at this |
@dipeshmsft: I found the issue, there seems to be a problem when loading a native dll from a local path which depends on another dll in a project that uses "DllImportSearchPath.AssemblyDirectory | DllImportSearchPath.System32" as the default search path. AFAIK it should work but for some reason it doesn't. This default search path is added to all WPF projects here: wpf/eng/WpfArcadeSdk/tools/ExtendedAssemblyInfo.props Lines 16 to 20 in 81019fc
The reason it doesn't fail in production is because the .Net host has custom search paths that includes the WindowsDesktop pack which contains the dlls. It's not specific to WPF, I was able to repro in a barebones project and I'll open an issue in dotnet/runtime to see if it's the expected behavior or not (Or maybe I misunderstand the bug). In the meantime, I'll open a PR to workaround this issue in test projects. |
Fixes #10250
Description
This fixes the flaky tests which was caused by the native dependencies sometime not being built before the test projects. This PR adds an explicit reference to the projects of the native dependencies to make sure that it's built in the right order and also adds some protection by including the files explictly instead of using file globbing to make the build fail with an error which contain the path of the file missing instead of the tests failing to start, which improves the build logs in case of error.
Customer Impact
None, tests only.
Regression
No.
Testing
Did a bunch of local builds and it fails on 1 out of 5 build before this PR and I did ~10 local builds with this PR and it didn't failed. I also validated the build order in the build log.
Risk
Low to none, tests only.
/cc @JeremyKuhne since you worked in that area too lately.
Microsoft Reviewers: Open in CodeFlow