microsoft / testfx

MSTest framework and adapter
MIT License
782 stars 259 forks source link

Proposal: UIDataTestMethod #740

Open marcelwgn opened 4 years ago

marcelwgn commented 4 years ago

The DataTestMethod attribute is really helpful and makes writing tests a lot easier. However when developing tests that need to run on the UI thread, we have to use the UITestMethod attribute. This however leaves us with having to implement data driven tests ourselfes. Having a UIDataTestMethod attribute would be great to have to fill the gap between UI tests and non UI tests.

AB#1408166

nohwnd commented 4 years ago

Maybe you could PR it?

marcelwgn commented 4 years ago

Where would this actually be contributed to? Does UITestMethod even live in this repository?

Rosuavio commented 3 years ago

@chingucoding it does live in this repo. https://github.com/microsoft/testfx/blob/ec18af6f90c272f68f48d9d0b94c71b8e276c7a0/src/TestFramework/Extension.UWP/UITestMethodAttribute.cs#L12-L43

Also on a similar note, I am working on the Windows Community Toolkit and we make heavy use of UITestMethod either creating this UIDataTestMethod would be very useful or maybe a better solution would be to make using UITestMethod and DataRow together work, rather than make a new attribute?

Currently using those attributes together causes this rather obscure error.

marcelwgn commented 3 years ago

Oh that's interesting, thanks for pointing that out @RosarioPulella!

I'm also not sure what the best move forward would be. I think having a UIDataTestMethod would be best for API simplicity since it extends on an existing pattern. I also got the issue you linked often and was confused what was happening. This error message should definitely be updated.

marcelwgn commented 3 years ago

@nohwnd What do you think is the best way forward here?

Rosuavio commented 3 years ago

I think having a UIDataTestMethod would be best for API simplicity since it extends on an existing pattern.

I am not in tune with a lot of the patterns of this API, but I see a lot of test methods decorated with DataRow and a simple TestMethod and it works fine. It also seems like there is an existing pattern to compose DataRow with other attributes than DataTestMethod.

Either way it goes, I would like something to open up for using DataRow in UI tests.

marcelwgn commented 3 years ago

Looks like you have a better view of how the properties work together. If DataRow works with normal TestMethod, why exactly is there a DataTestMethod attribute though? I though you need to have those and otherwise it wouldn't work.

Totally agree that no matter what we settle on, the situation needs to change. Having no DataRow support in UI tests is quite annoying.

michael-hawker commented 3 years ago

@chingucoding they DataTestMethod used to be a separate thing, but they merged them and just left the old one there for compatibility for folks. It should probably just be marked Obsolete so folks could move off of it and they could remove it in the future.

We hit the same problem with async as well, but using a TestMethod to just dispatch to the UI thread first things seems to be working. Same for doing a similar thing for DataRow as well. It's just a bit more verbose.

Haplois commented 3 years ago

DataTestMethodAttribute has no functionality, we keep it there for compability. It's same as TestMethodAttribute. Instead of adding a new method, we should fix the bug in UITestMethod.

marcelwgn commented 3 years ago

I would like to take a look at this.

Haplois commented 3 years ago

@chingucoding go for it. Thanks.

michael-hawker commented 3 years ago

@Sergio0694 your dispatcherqueue extensions in the toolkit test if it needs to be dispatched first, eh? So it's probably a similar type of helper needed in DataRow, so if it's used with the UITestMethod test, then it'll just dispatch the call as needed to where the test is running? I think?

Sergio0694 commented 3 years ago

"your dispatcherqueue extensions in the toolkit test if it needs to be dispatched first, eh"

That is correct. Though note that that was just meant to be a small performance optimization for when the extension was needlessly called, and it should not result in any functional changes compared to someone just always using just TryEnqueue. That is to say, the fact that those extensions do that should not (I think) affect how or when they're being used. It's just that, whenever they're used, they also try to save some more memory and CPU time if they can, otherwise just forward to TryEnqueue as usual 😄

marcelwgn commented 3 years ago

Was trying to test this inside the project by adding a UWP test app and have the use the libraries from the repository, however I can not build the project as it fails with the following error message:

1>CSC : error CS7032: Key file 'D:\Projects\testfx\scripts\build\key.snk' is missing the private key needed for signing

Maybe you guys know more about this or know a way to work around this?

michael-hawker commented 3 years ago

@chingucoding I believe that path is stored in one of the project or manifest files, you should just be able to remove that line so it doesn't try and use the non-existent snk file?

marcelwgn commented 3 years ago

@chingucoding I believe that path is stored in one of the project or manifest files, you should just be able to remove that line so it doesn't try and use the non-existent snk file?

If I do that, the issue get's even worse as other projects also fail to build...

michael-hawker commented 2 years ago

@chingucoding did you get anywhere with this? We're hitting this again with Toolkit Labs for Windows, so it's a bit of a pain. It requires us to use TestMethod and then Dispatch to the UI Thread in each of our tests to start, instead of being able to just have that be handled automatically by the test harness. (As we're trying to await the content to test to be loaded.)

@pavelhorak saw this was added to a milestone is it something your team was also thinking of looking at? I tried to write our own TestMethod wrapper using DispatcherQueue for UWP as well, but ran into the same issue. The test app just hangs in a deadlock as soon as the first await is hit in test initialization or test code. (I'd also suggest renaming this issue to "support async methods with UITestMethodAttribute"; compared to the initial request and discussion, the end result is this missing feature.)

Could someone explain the underlying issue with the state machine of why this is currently not supported by the UITestMethodAttribute? @Sergio0694 I may ask your thoughts on this in the morning.

https://github.com/microsoft/testfx/blob/5862d22be4616058d5f78e0b46737d3af8f42956/src/TestFramework/Extension.UWP/UITestMethodAttribute.cs#L12-L30

mrlacey commented 2 years ago

@chingucoding I believe that path is stored in one of the project or manifest files, you should just be able to remove that line so it doesn't try and use the non-existent snk file?

If I do that, the issue get's even worse as other projects also fail to build...

This sounds like a key is needed for strong naming but the default .gitignore was used and this prevents .snk files from being checked in. (This is something I frequently see with projects on GitHub.) Creating a new key with that name/path will probably fix it. It won't fix it if the public key of the missing file is used somewhere but if that was the case I'd expect this to be a bigger issue for more people working on the codebase.

michael-hawker commented 2 years ago

I tested, and I think the UITestMethod for WinUI 3 has the same issue. I was hoping since it's already using the new DispatcherQueue that it may be able to work.

What I don't understand is why the regular TestMethod attribute supports this? It's execute is just calling Invoke directly. Is it the extra GetAwaiter().GetResult() pattern that's locking the thread pool as the async/await pattern isn't being used by the test running itself?

michael-hawker commented 2 years ago

@dotMorten, @azchohfi mentioned you may have found a workaround/solution to this by tweaking a custom attribute? Do you have that somewhere or was it in regard to something else?

dotMorten commented 2 years ago

@michael-hawker There's a few ways to do it. WinUIEx has a test tools package that uses a code-generator to pull it off with a simple tag. But you're right it can also be done with a custom TestMethodAttribute - I'll have to dig that out if you need it. I definitely don't use the mstest provided UITestMethod since it doesn't support async tests - my solution does. But the simplest way is to just use a dispatcher to switch to the UI thread like I'm doing in this WPF helper method here: https://github.com/dotMorten/UniversalWPF/blob/main/src/UnitTests/Framework/UIHelpers.WPF.cs#L55-L59 Then it's just a matter of awaiting that call like this: https://github.com/dotMorten/UniversalWPF/blob/main/src/UnitTests/RelativePanelTests.cs#L19-L20

abdes commented 1 month ago

Any update on this issue please?

Evangelink commented 1 month ago

Hi @abdes,

No update so far. I'll loop in that request for our next sprint and keep you posted if that gets approved.

michael-hawker commented 1 month ago

@Evangelink happy to discuss what we've done in the Windows Community Toolkit in this space. We've done some source generator stuff for hooking in to XAML pages mostly.

Though for DataRow we still use the DataTestMethod and just manually dispatch to the UI thread (which is all the UITestMethod really does anyway): https://github.com/CommunityToolkit/Windows/blob/main/components/Triggers/tests/ControlSizeTriggerTests.cs

dotMorten commented 1 month ago

I have a slightly different approach I'm more than happy to share as well.

However the best solution would be to have an Task<TestResult> ExecuteAsync alternative to the synchronous TestResult Execute method available on TestMethodAttribute so we can get rid of the Task.Wait code in the test executor that is the root of the problem to making things async to get safely to the UI thread and back.

Evangelink commented 1 month ago

@dotMorten That's on the plans, sadly it needs to flow everywhere in MSTest which is causing many public methods to be impacted so we have to group all these changes for v4. I do hope this is something we will be able to plan for next year.

abdes commented 2 weeks ago

I have a slightly different approach I'm more than happy to share as well.

However the best solution would be to have an Task<TestResult> ExecuteAsync alternative to the synchronous TestResult Execute method available on TestMethodAttribute so we can get rid of the Task.Wait code in the test executor that is the root of the problem to making things async to get safely to the UI thread and back.

Totally agree that the [UITestMethod] does not support async test methods is just soooo hindering. Basically reduces the unit testing to testing dependency properties, which is useless in most of the cases.

I ended up, writing tests, using an approach similar to yours @dotMorten , with the addition of a helper, from CommunityToolkit that awaits for the UI to render so that the control is fully realized before asserting stuff. That way we can test a lot of the visual aspects of the control as well.

Then I banged my head against the wall for a day to figure out how to test visual state transitions, to finally find out that it is a s simple as attaching a custom VisualStateManager to the the element that has the VisualStateGroup.

I don't know, but I just hope that one day we don't have to do that and we just get a nice [UITestMethodReallyWorks] attribute, because all of the boilerplate you do and I do is totally source generatable... Or, another day, I will be so bored, I will write the source generator for it 😄

Love the work done by @Evangelink , but we need more for this WinUI thing to start being productive, and Micro$oft is spending the dollars on Aspire 😄