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

Support parallel test execution #60

Closed
kyriee opened this issue Feb 28, 2013 · 35 comments

Comments

@kyriee
Copy link

commented Feb 28, 2013

Parallel test execution is not currently supported.
If it turned on, ExpectationException (Call to non configured method) errors appear :(

@adamralph

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2013

Can you please take a look at http://hmemcpy.com/2012/12/running-multithreaded-unit-tests-with-fakeiteasy/ and see it solves your problem? /cc @hmemcpy

@kyriee

This comment has been minimized.

Copy link
Author

commented Mar 6, 2013

Nope, I've double-checked, seems this doesn't work for me.
I think, the configured calls are not even been invoked. Probably, all calls to the "CallsTo" method should be in a critical section.

@adamralph

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2013

OK. I guess the root cause may be that we have statics in FIE which are not thread safe. The solution which I suggested above should take care of multi-threading within a single test method (although I don't think that's something which is appropriate for a unit test) but perhaps it doesn't for the case of execution of many test methods in parallel.

Ultimately we may need to review all statics in FIE for thread safety, but this could be quite an undertaking. In order to solve the particular issue you are facing, we need to be able to reproduce the problem. Would you be able to supply code (bare minimum) and instructions for how to reproduce the issue?

@sampalmer

This comment has been minimized.

Copy link

commented Nov 28, 2013

I have the same sort of problem using MSTest configured to run tests in parallel (max parallelism of 2). Being a threading issue, it happens intermittently, and I don't have the time to set up threading primitives to ensure that the timing causes a consistently reproducible symptom of the problem. However, here's an example where it affects me:

logger.CallsTo(l => l.Log(A.That.Contains("Home"))).MustHaveHappened();

Produces:

Assertion failed for the following call:
Example.Project.ILogger.Log()
Expected to find it at least once but found it #0 times among the calls:
1: Example.Project.ILogger.Log(description: "Home Page")

This happens when I run it at the same time as 4 other similar/related tests.

Looks like the easiest work-around at this point is to just disable parallel test execution, unfortunately.

@Mpdreamz

This comment has been minimized.

Copy link

commented Jan 3, 2014

Same here using NCrunch's parallel execution.

I'm reusing Fakes by assigning them to a private readonly fields which is what probably does it. I now just branch every test into its own subclass which solves it for me in both the NCrunch and NUnit runners. It's a little more effort but well worth it to keep the parallel execution.

@adamralph

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2014

Interesting. So far no one's been brave enough to step up and attempt the static thread safety overhaul. We live in hope though 😉

@adamralph

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2014

Gave this a P2 since we have a recent user comment and I think this is a good enhancement.

@blairconrad

This comment has been minimized.

Copy link
Member

commented Aug 13, 2014

@kyriee, @sampalmer, do you know whether, like @Mpdreamz, you were reusing fakes by assigning them to fields on the test classes? I can certainly see that being a problem, and may be impossible to work around. I don't think we would ever be able to commit to supporting parallel test execution in such a scenario.

If the affected tests weren't "sharing fakes", then we do have a deeper problem (and I am with @adamralph: I would not be surprised to learn that there's some state-sharing within FakeItEasy) that could benefit from investigation. This does seem tricky, though, especially since FakeItEasy uses the "Just Create One" pattern in preference to traditional singletons or static members, and I think this will make the hunt more difficult.

@adamralph

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2015

Now that xunit 2.0 has gone stable, and runs tests in parallel by default, this is a more pressing issue.

@adamralph adamralph added P1 and removed P2 labels Mar 18, 2015

@blairconrad

This comment has been minimized.

Copy link
Member

commented Mar 18, 2015

I didn't realize that XUnit was switching to running tests in parallel by default. An interesting choice. Given most of the tests I've seen in the wild, this would've been disastrous. Then again, the ones I usually see are written in NUnit, and I understand the patterns are different.

I worry that we'll be able to make this change in a timely fashion. In particular (and without diving into the code) I think the scopes support will be problematic.

@adamralph

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2015

Actually I should qualify that: the default behaviour is to run all tests within a class in parallel. There is also an option to run all tests in an assembly in parallel.

@adamralph

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2015

A sledge hammer approach to scopes (and perhaps the only feasible one) is to block any threads trying to create a new scope until the current one is disposed. This would effectively serialise tests which would otherwise be parallel.

@blairconrad

This comment has been minimized.

Copy link
Member

commented Mar 18, 2015

At least a new non-nested scope. Which I think we support. Although I shouldn't comment without taking more time to look at the code than I'm going to this morning.
I don't hate that approach, if it's all we can do. Or a stop-gap.

@aarondandy

This comment has been minimized.

Copy link

commented Apr 3, 2015

Here is a related issue: xunit/xunit#357

A large portion of my tests are and also use async methods so this is an issue for me as well. For now I am marking my test assemblies with [assembly: CollectionBehavior(DisableTestParallelization = true)] (note that the spelling of the property may differ between versions of xunit).

@blairconrad

This comment has been minimized.

Copy link
Member

commented Apr 3, 2015

Very preliminary testing supports the idea that the That.Matches is one failure point in the thread-unsafety. I have failing test cases based off https://gist.github.com/StefanBilliet/982ef274c4303235ecd7. Converting the tests to non-Tasks exhibits the same behaviour, but when I drop the That.Matches, I get no test failures.

I suspect that the static ArgumentConstraintTrap.trappedConstraints is the culprit. Will investigate more later in the weekend (family events interfere).

@GraemeBradbury

This comment has been minimized.

Copy link

commented Sep 23, 2015

To add to this. We've started seeing an issue where calls to faked methods are returning Dummies even though setups have been done.
eg: A.CallTo(...).Return(...) are being ignored.

Current instances seen have always been setups in xBehave [Background] steps so not yet sure if it's a FakeItEasy issue or a xBehave one.

@adamralph

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2015

@GraemeBradbury I'd hazard a guess that it's almost certainly a FakeItEasy problem. xBehave 2.0 is well isolated in terms of thread safety.

@blairconrad

This comment has been minimized.

Copy link
Member

commented Sep 23, 2015

@GraemeBradbury this is with xBehave 2.0, yes?
Which version of FakeItEasy? Have you a small sample you can share?

@GraemeBradbury

This comment has been minimized.

Copy link

commented Sep 23, 2015

Nuget Package versions:
xBehave 2.0.0
xUnit 2.0.0
FakeItEasy 1.25.3

I'm trying to create a minimum example test. The tests that exhibit the problem are all integration tests (faking out the file system). Will update as soon as I have something.

@blairconrad

This comment has been minimized.

Copy link
Member

commented Sep 23, 2015

Ah. The FakeItEasy 2.0 betas definitely have some threadsafety improvements.
If your problems go away when all tests are run single-threaded under 1.25.3, I'd be inclined to move on to how they behave under the latest 2.0.
Speaking for myself, I've no interest in making 1.x.y threadsafer…

@adamralph

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2015

👍 for giving it a try with [assembly:Xunit.CollectionBehavior(MaxParallelThreads = 1)]

@apacha

This comment has been minimized.

Copy link

commented Oct 14, 2015

Is there a roadmap when Version 2 will be released? We are looking forward to the support of parallel test-execution because our Jenkins is constantly turning red because of FakeItEasy, although we have tried to configure thread-safety during creation of the fake object, we now get race-conditions during A.CallTo(...).MustHaveHappened() - assertions because of parallel test-execution.

@blairconrad

This comment has been minimized.

Copy link
Member

commented Oct 14, 2015

@apacha, we have no dates set. Since we work on FakeItEasy in our spare time, and have outside interests, it's nearly impossible to predict. The other answer is "as soon as we can", as we'd enjoy publishing the changes that we have and then resuming our regular cycle of frequent small releases.

I am surprised your tests are running in parallel, and have commented on your StackOverflow question. Perhaps we should meet up over there to discuss how your tests are running in parallel to begin with.

In the meantime, there are no "threadsafety" issues planned for 2.0, so 2.0.0-beta010 has everything that 2.0 is likely to get. You could try that release.
Or, if you can help identify a particular problem not addressed by 2.0.0-beta010, raise it and let's see what we can do.

@apacha

This comment has been minimized.

Copy link

commented Oct 14, 2015

Thanks for the fast response. We will try 2.0.0-beta010 to see if this resolves our issues.

@blairconrad

This comment has been minimized.

Copy link
Member

commented Oct 14, 2015

👍 @apacha. I'd be keen to hear how it works out for you.

@apacha

This comment has been minimized.

Copy link

commented Oct 15, 2015

I've tried 2.0.0-beta010 today and ran our test-suite (approx. 800 tests) about ten times with the newer version of FakeItEasy. My impression is that we get fewer race-conditions during creation and assertion, but they still persist ("Enumeration was changed" exceptions and A.CallTo()-assertion that were lost). We will investigate further how the tests are running on the Jenkins.

@blairconrad

This comment has been minimized.

Copy link
Member

commented Oct 15, 2015

Very peculiar. I'd be happy to help take a look at whatever portions of the tests and code you can share, once you've a little more of a handle on what's going wrong.

@adamralph

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2016

Note, via @dennisdoomen, https://twitter.com/ddoomen/status/685108134269026304?s=09

Just make sure you don't use Fake.CreateScope. That'll break it.

@blairconrad

This comment has been minimized.

Copy link
Member

commented Jan 7, 2016

Yup, I also refer readers to #484.

@adamralph

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2016

This has been taken care of in several other issues as part of 2.0, with the only known outstanding issue being #484.

If any other problems with parallel execution arise, we can deal with them as bugs at that time.

@erlis

This comment has been minimized.

Copy link

commented Jan 27, 2017

Team,

I see a few thread safety issues but all of them have been marked as closed.

I'm using version 2.3.3 and NUnit 3.4.1 running tests in parallel with the Parallelizable(ParallelScope.Fixtures) setting and I'm having FIE failing an expectection with message:

Assertion failed for the following call: Common.Logging.IAppLogger.Error("DeleteEmployeesByX DeleteEmployee failed. EeId: {0}", [<Ignored>]) Expected to find it exactly 2 times but found it #1 times among the calls: ....

Is this because the Ignored parameter?

The test passes if parallel execution is disabled.

Thanks,
Erlis

@blairconrad

This comment has been minimized.

Copy link
Member

commented Jan 28, 2017

Hi, @erlis. Thanks for bringing this up.

As you say, all of our known thread safety issues have been closed. It's entirely possible that we've missed something.
I doubt the Ignored is the culprit here (but anything's possible).

It's hard to do much debugging without more information. Do you have a minimal test case that reproduces the issue and that you could share? Alternatively, I can try to whip something up, but I won't be able to look at it for at least 2 days, and there's no guarantee that I'll be trying the right thing.

One thing I will ask, though. You're not sharing Fake objects between tests that are running in parallel, are you?

@erlis

This comment has been minimized.

Copy link

commented Feb 14, 2017

hi @blairconrad,

I found that the error I was having was not because FakeItEasy but an actual bug on my code, I'm testing the parallel execution and so far everything is working as expected, If I find a problem.. should I open a new bug or add to this one?

Regards,
Erlis

@adamralph

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2017

@erlis yes, if you find any problems, please open a new issue. Many thanks.

@blairconrad

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

@erlis, sorry I never got back to you. Work and non-work things interfered (and somewhere in the back of my head probably a feeling that I'd never be able to reproduce your problem with the info I had). I'm glad things worked out for you, and that FakeItEasy, however indirectly, helped you find the bug in your code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.