-
Notifications
You must be signed in to change notification settings - Fork 73
Added [LabsUITestMethod] attribute and source generator #156
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
Conversation
…oved control lifetime.
@Arlodotexe should we add an example test to the template as well to show this being used for folks? |
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.
Few small things and some suggestions for more tests to shore things up.
...CommunityToolkit.Labs.Core.SourceGenerators.LabsUITestMethod/Extensions/ISymbolExtensions.cs
Show resolved
Hide resolved
.../CommunityToolkit.Labs.Core.SourceGenerators.LabsUITestMethod.Tests/LabsUITestMethodTests.cs
Outdated
Show resolved
Hide resolved
/// <returns>When UI is loaded.</returns> | ||
protected Task SetTestContentAsync(FrameworkElement content) | ||
{ | ||
return App.DispatcherQueue.EnqueueAsync(() => |
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.
Oh, also really confused why the extension isn't working as it's a bit odd for us to re-implement this. Can you sync with @Sergio0694 quick sometime for 10 minutes on a call and trying and diagnose/go through the issue?
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.
I'll set that up in the morning, see if we can pick out exactly what went wrong. I can't repro the issue yet, but we can use the code from before this PR.
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.
Is it only this overload taking a Func<Task<T>>
that's causing issues, or all of them?
Looks like a test failure isn't getting caught by the MSTest infrastructure at the moment now with the new attributed method? We should have a case to try where use the attribute to expect a known exception and then throw that exception and make sure that works... |
Hmm, the error may be getting eaten in the CI as well, as I just see the general abort message as well: https://github.com/CommunityToolkit/Labs-Windows/runs/6914398636?check_suite_focus=true#step:15:227 - hopefully we can fix the error bubbling, and then get more info out of the CI about the failure... though I'll be sad if we can't run the input injection APIs in the CI... 😥 |
That's odd. I suppose we should add this to #160, but that means this PR can't be closed until that one is. |
@michael-hawker Sergio and I weren't able to reproduce the issue with the EnqueueAsync extension in the toolkit. It's been reverted and test now succeed and fail as expected. Feel free to rebase your branch as needed. |
> Test fail for no reason |
@Arlodotexe don't think the build ran as there's conflicts to resolve? |
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.
Looking good, will see how the CI does (and I'll try on my other branch to see what happens too). Just a few file-scoped namespaces that were missed.
.../CommunityToolkit.Labs.Core.SourceGenerators.LabsUITestMethod.Tests/LabsUITestMethodTests.cs
Outdated
Show resolved
Hide resolved
...nityToolkit.Labs.Core.SourceGenerators.LabsUITestMethod/Diagnostics/DiagnosticDescriptors.cs
Outdated
Show resolved
Hide resolved
I ran into some issues with Visual Studio refusing to build after resolving conflicts, complaining about missing types. Should be good now. |
.../CommunityToolkit.Labs.Core.SourceGenerators.LabsUITestMethod.Tests/LabsUITestMethodTests.cs
Show resolved
Hide resolved
My other PR helped to exercise this a bit more too. I'll do a quick check locally, but then I think we'll be good! |
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.
Yay, I see the failures now in VS! 🎉🎉🎉
Seems to be working great. Awesome job! Thanks @Arlodotexe!
…lkit#156) * Created UIControlTestMethod generator + unit tests * Added new source generators to template * Migrated existing tests to new source generator attribute * Refactored to pull control instance directly from method params. Improved control lifetime. * Cleanup outdated comments * Added mandatory content cleanup for UI tests * Fixed diagnostic ID to match project name initials * Fixed source generator to only check public constructors for LUITM0001 * Fixed test setup/cleanup invoking on the wrong thread * Added new source generator to individual project solutions * Reverted to toolkit extension for dispatcher EnqueueAsync * Remove whitespace * Fixed non file-scoped namespaces * Added missing file header * Removed unused using directives
Background
Closes #143
This PR adds the
[LabsUITestMethod]
attribute and source generator. When the source generator sees this attribute, it will:Basic usage
Synchronous, with control:
Synchronous, without control:
Asynchronous, with control:
Asynchronous, without control: