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

StaFact facts breaks the concurrency limitations of Xunit #53

Closed
lg2de opened this issue Jan 31, 2021 · 11 comments · Fixed by #55
Closed

StaFact facts breaks the concurrency limitations of Xunit #53

lg2de opened this issue Jan 31, 2021 · 11 comments · Fixed by #55

Comments

@lg2de
Copy link
Contributor

lg2de commented Jan 31, 2021

Xunit has an internally implemented limitation of tests to be executed concurrently. This is implemented based on fixed number of threads executing the tests.

All Fact implementations of StaFact packages creating its own thread, e.g. UIFact, will break the Xunit limitation because the Xunit thread is released after UIFact test execution has been started. I think this causes unexpected high CPU load while test execution resulting in accidental failed test execution, e.g. fluentassertions/fluentassertions#1391 and https://ci.appveyor.com/project/dennisdoomen/fluentassertions/builds/37160306

I've created and attached a sample project showing the problem. For demonstration I've configured the limitation to 2 concurrent tests. All standard Fact tests completing successfully. Most UIFact tests will fail because the maximum number of tests is exceeded.

StaFactTests.zip

@aesalazar
Copy link

We occasionally see our tests fails or even hang on our build servers which is mentioned in one of those threads. Have you seen this as well?

@lg2de
Copy link
Contributor Author

lg2de commented Feb 4, 2021

Yes, they have been failed if they rely on timing (assertion of execution time).
They did NOT hang, at least we haven't noticed that so far.

@AArnott
Copy link
Owner

AArnott commented Feb 5, 2021

Thanks for reporting. Any idea how we would fix this? I have no experience with the xunit parallel execution throttle. The xunit methods are mostly async, so it seems designed that tests not always block a thread, so if xunit's throttling assumes the test blocks a thread, that already seems broken without UIFact's help. Can you repro this problem with any [Fact]-based async Task returning method that yields right away with .ConfigureAwait(false)?

@lg2de
Copy link
Contributor Author

lg2de commented Feb 6, 2021

I think it should be ok to block the xunit thread while UIFact thread (and all other thread in this extension) is executing. Synchronous thread will "block" the thread too.
Let say, the UIFact thread replaces temporary the xunit thread.

Can you repro this problem with any [Fact]-based async Task returning method that yields right away with ConfigureAwait(false)?

I could not really understand your question.
And, what is the background of this question?

@AArnott
Copy link
Owner

AArnott commented Feb 7, 2021

what is the background of this question?

I am trying to establish how xunit throttles test runs when tests are asynchronous. Because whatever that mechanism is, we should reuse it for UIFact.

@lg2de
Copy link
Contributor Author

lg2de commented Feb 12, 2021

Xunit starts as many threads as tests should be in parallel. All testing work is distributed to these worker threads.

With the additional threads of StaFact the mechanism cannot be reused, I guess.
Instead I think the the Xunit thread should be blocked while StaFact thread is running.

Maybe @xunit can help?

@lg2de
Copy link
Contributor Author

lg2de commented May 15, 2021

@AArnott , when do you plan to create a new release with this issue fixed?

@AArnott
Copy link
Owner

AArnott commented May 15, 2021

I'll try to get it out later today.

@AArnott
Copy link
Owner

AArnott commented May 16, 2021

@lg2de
Copy link
Contributor Author

lg2de commented Jan 21, 2022

Hey @AArnott, could you please create a new public release?
I have a new project which must not use alpha or beta releases.

@lg2de
Copy link
Contributor Author

lg2de commented Jan 21, 2022

We found another issue... :(
Please wait with the release.

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