Skip to content
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

Running all tests within a class on the same thread? #54

Open
eriove opened this issue Apr 1, 2021 · 12 comments
Open

Running all tests within a class on the same thread? #54

eriove opened this issue Apr 1, 2021 · 12 comments

Comments

@eriove
Copy link

eriove commented Apr 1, 2021

Would it be possible to add an option to run all tests within a class on the same thread? Right now all tests run on an STA thread but not on the same STA thread. I ran into problems where different UI components end up on different threads which causes failures.

See this merge request for an example of a workaround.

The behavior can be observed by making the following change to the constructor of StaFactTests.

    public StaFactTests(ITestOutputHelper testOutputHelper)
    {
        Assert.Equal(ApartmentState.STA, Thread.CurrentThread.GetApartmentState());
        testOutputHelper.WriteLine("Currently on thread {0}", Thread.CurrentThread.ManagedThreadId);
    }

Running the test will print the following lines (with varying numbers):

Currently on thread 22
Currently on thread 23
Currently on thread 24
@lg2de
Copy link
Contributor

lg2de commented Apr 1, 2021

I think, this issue is related to #53.

@eriove
Copy link
Author

eriove commented Apr 1, 2021

I think, this issue is related to #53.

I agree. It seems like this issue would be solved if #53 was solved.

@AArnott
Copy link
Owner

AArnott commented Apr 2, 2021

@eriove Why is it important to reuse a thread for all tests in a class? Are your tests sharing objects with each other? That tends to be discouraged so that one test doesn't discretely lead to another test's failure.

@eriove
Copy link
Author

eriove commented Apr 2, 2021

I think the source of my problem is that WPF applications usually have one UI thread, hence there might be assumptions in the WPF code or WPF component code that relies on that fact.

In this merge request you can see an example of that. I create a new DockingManager in each test (and there is no shared state between the tests). Despite this the ContextMenu (which isn't explicitly used in the test) gets reused. Since it was created on another thread it will throw when accessed on the current test's thread.

This StackOverflow question seems to suggest that there is a caching mechanism for ContextMenus.

I'm not quite sure where to solve this. When I'm back at work on Tuesday I will see if I can do some experiments with styles or just set the context menu explicitly to get around my particular problem. But this seems like a problem that could occur for other people and solving it here would solve it for everyone without having to track down what UI element that gets cached.

But I'm not sure, maybe the libraries should be left alone and I should just dig further in my own tests...

@AArnott
Copy link
Owner

AArnott commented Apr 2, 2021

Aside: If you're testing WPF code, are you using [StaFact] or [WpfFact]? (this library defines both)
I don't think that switching would fix this problem, but in general [WpfFact] does a better job at emulating what WPF expects.

"Fixing" this would mean that all tests have to run sequentially instead of in parallel, which could be a huge takeback for some folks with many tests. The test title qualifies it with "within a class" but if ContextMenu really is retaining some state behind the scenes, wouldn't fixing it require that all tests run on the same thread, whether they belong to the same test class?

@eriove
Copy link
Author

eriove commented Apr 12, 2021

Sorry for taking so long to reply. It has taken a while to process this. I've been using [WpfFact] and as you thought it does not help.

You are right about needing to run all tests sequential to resolve the problem and that that would be a bad idea for projects with many UI dependent tests. I did not think about that. I'm lucky enough to only have 10 to 20 tests that needs to run on a STA-thread.

I've created a minimal project that reproduces the problem and the problem is that a style contains a ContextMenu. Styles are cached in a static cache so the context menu will be re-used between tests. By changing from StaticResource to DynamicResource the problem disappears.

I imagine that this wouldn't be an unusual problem. Do you have any other suggests on how to solve the problem?

@AArnott
Copy link
Owner

AArnott commented Apr 17, 2021

Thanks for sharing the repro project. I think I need to re-familiarize myself both with this project and how xunit does thread management since this issue and #53 may have overlap so I'd want to look at them together. I don't have time now to investigate though, but I'll keep it active.

@eriove
Copy link
Author

eriove commented Apr 17, 2021

No problem. My merge request for changing from static to dynamic resources was merged into AvalonDock today so the immediate problem is solved. If nothing else this thread might help others facing the same problem.

@AArnott
Copy link
Owner

AArnott commented Apr 27, 2021

Ok, so now that #53 is fixed and didn't impact this, I'm ready to revisit. Since so many other WpfFact tests don't hit this, and you have a workaround, we might entertain not "fixing" this here. If we did, I'd want to look for a way for a test project, class, or method to "opt in" so that tests that do not require sharing a single thread are not unnecessarily slowed down.

One obvious idea is to add an optional named argument to the WpfFactAttribute to specify that a single thread must be shared with all other tests that also set this boolean property. But maybe that's too tedious to maintain on every single appearance of the attribute. Would an assembly or class-level attribute be more appropriate?

@eriove
Copy link
Author

eriove commented Apr 28, 2021

I like the idea of an optional argument to WpfFactAttribute. It seems like this is an uncommon problem, so there shouldn't be many tests that need it. Optional arguments are much easier to discover than assembly level attributes and configuration files. Since it might be a couple of years between the times a single developer needs this, it is better to see the optional argument than to google for the error message. Especially since searching for dispatcher exceptions will give you pages of explanations for beginners on how to use a dispatcher.

@magol
Copy link

magol commented May 4, 2021

I would prefer to use an assembly or class-level attribute as an alternative to a optional parameter. ALL my tests require that they run on the same thread, so it would have been a pain in the ass to have to add a parameter to each test.

@AArnott
Copy link
Owner

AArnott commented May 15, 2021

I think at least class-level makes sense. If only a couple tests need it, people can put those in another class. I'm not against a test-level attribute as well, but maybe that can be based on demand.
Is anyone interested in trying out a change and sending a PR for review? I'm not promising to take iteration 1, of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants