Thread ID changes between test and *Invocation events when the test timeout set
jcdkount opened this issue · 27 comments
Using the attached code, run the Bug.xml file.
EXPECTED: beforeInvocation, afterInvocation should always be on the same thread as the test.
ACTUAL: When you change the timeout in the annotation transformer (IAnnotationTransformer), the before and after Invocation events are on a different thread.
NOTE: In testing, I noticed that if I set the timeout by the @test annotation, it also has the same problem, but I suspect it happening at run time might make this harder to patch, so I left in the more complex case.
Example output:
beforeInvocation ; Expected SAME Thread: 11 - pool-1-thread-1
myExampleTest ; Expected SAME Thread: 12 - TestNGInvoker-myExampleTest()
afterInvocation ; Expected SAME Thread: 11 - pool-1-thread-1
Possible ways to fix this:
- Make the thread the same even if the timeout changes.
- ITestResult or IInvokedMethod include the thread id or Thread object which is stored when the the test method is executed.
For further detail on the discussion of this issue, see https://groups.google.com/forum/#!topic/testng-users/W3lQsykKa3w
The reason for this is that when you set a time out on a test method, that method gets run on a one-off executor so that the time out can be applied on it.
I once looked into fixing this but this changehas a lot of deep reaching consequences that I don't really have a clear resolution for.
Glad to know there's an open issue for this! This is currently messing up with my logging (log4j). Is there any place where I can do some code before the test starts? Somewhat like "beforeInvocationInsideTestNGInvoker"?
I've done a bit of research. I saw that on some comment you said that not using a new thread for each test run when there's a timeout would be a problem (I don't know why but hey you're the expert). What I don't get is why are you not calling the "run" method of an IHookable if there's a timeout also.
Does the timeout protection break SO much the whole thing?
not using a new thread for each test run when there's a timeout would be a problem (I don't know why but hey you're the expert)
Because timeout is managed by a executor which is a simple way to manage timeout. But we are open to another solution.
What I don't get is why are you not calling the "run" method of an IHookable if there's a timeout also.
Well, I found out a workaround. I renamed the thread name for something that contained "TestNG" and that allowed me to keep the tests under the same thread:
Thread.currentThread().setName("TestNG-" + testname);@cbeust , until the fix for this bug is not comming, do you have any idea what kind of implications would this mean if I use this hack together with a timeout on @Test methods?
The problem here is that I use too many features probably heh, retryanalizer, dataproviders, timeouts, etc all at the same time heh.
@juherr Sorry I was writing my last comment when you sent your answer. I double checked, that "invokeHookable(...)" is only used inside an if condition like:
// If no timeOut, just invoke the method
if (MethodHelper.calculateTimeOut(tm) <= 0) {
[...]
MethodInvocationHelper.invokeHookable(instance,
parameterValues, hookableInstance, thisMethod, testResult);
[...]
}
else {
[...]
MethodInvocationHelper.invokeWithTimeout(tm, instance, parameterValues, testResult);
}
}
And that "invokeWithTimeout" does NOT call the run method in iHookable :(
The call is not direct, but: "invokeWithTimeout" -> "invokeWithTimeoutWith*Executor" -> "InvokeMethodRunnable#run()#runOne()" -> "invokeHookable" -> "hookable.run(....)"
I just tested out... I was using TestNG 6.9.4 but when upgraded to 6.9.5 and now it uses the IHookable run! I just compared both versions and you guys just added/fixed that. Thanks for pointing me to the right direction!
I use a log4j setup that logs into files, one per thread. Your thread magic when there's a timeout was breaking that logging set up heh.
I use a log4j setup that logs into files, one per thread. Your thread magic when there's a timeout was breaking that logging set up heh.
I don't see any solution to fix this issue for the moment.
reference.
blocker for reportportal/client-java-core#6
This issue is long been pending can we expect fix in tesng 7.0.0
Not sure but we can try
dependsOn is also creating new thread and there is a fix for that using -Dtestng.thread.affinity=true.
Is this works for the timeout as well?
@balrajHostAnalytics I don't think so. If the fix is possible then it will be the default behavior.
We are also face this issue mith @Before-/AfterMethod. We currently rely on the use of ThreadLocal. Using a time-out breaks this, as the @Before-/AfterMethod uses a different thread.
@cbeust @juherr @krmahadevan Hi. What stops us from fixing this?
I can try to prepare a pull request if we all agree that listener's "before" and "after" methods should be executed in the same thread as the test itself. Do you agree that this is the right way to fix the problem?
@asolntsev - The reason why we are stuck with this issue is because of the timeout feature which causes TestNG to spawn a new thread and run the test method in that thread and the current thread is used to play the role of a monitor which will kill this newly spun off child thread if the timeout exceeds. Refer here
From the current implementation I personally don't see a way in which we can honour the thread affinity guarantee when a test method is coupled with a timeout.
If you have any ideas on how we can circumvent past this problem, please do share them as a PR
The idea is very simple: run listener's "before" and "after" methods in the same thread as test itself.
Sure. But running the before and after in the same thread as the test basically will pollute the timeout construct because the timeout is supposed to be applied only for the test method, but having them run as a single unit in a thread will perhaps violate this contract.
@krmahadevan There is an easy solution: change the contract!
Now timeout will be applied to the test WITH its "before" and "after" methods. It seems even logical to me.
@asolntsev - thats a change in functionality and to me it changes the semantics of timeout
@juherr what do you think ?
Also it will cause all the before and after listeners found on the classpath to be clubbed with the test and kind of deviate the functionality.
Also it will cause all the before and after listeners found on the classpath to be clubbed with the test and kind of deviate the functionality.
And... What's the problem? I don't think it's a problem.
If you want to keep the backward compatibility, there is also an easy solution:
we could invent a new interface for listeners, like INewTestListener with methods beforeTest and afterTest.
And these new methods beforeTest and afterTest will be executed in the same thread as test.
So all people who needs the same thread will implement this new interface INewTestListener.
And... What's the problem? I don't think it's a problem.
It depends on the sort of usecases that people are handling with in their own repositories. So I believe its going to be unfair to assume that a flip will not cause any issues to anyone else.
If you want to keep the backward compatibility, there is also an easy solution: we could invent a new interface for listeners, like INewTestListener with methods beforeTest and afterTest.
And these new methods beforeTest and afterTest will be executed in the same thread as test.
So all people who needs the same thread will implement this new interface INewTestListener.
Sure. We can consider introducing a variant of IInvokedMethodListener which would get invoked from within the same thread
I'm not sure to understand why a new interface will help.
Why not have a behavior parameter instead?
For me personally, the parameter also solves the problem.
But for a wider audience, the interface is better for 2 reasons:
- It allows people to implement both interfaces (someone might need them both, right?)
- for me, as an author of testing library (Selenide), it's much easier to implement the right interface than communicate to all my users that they need to change some configuration parameters.