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

Error handling in tests #771

Closed
leoxs22 opened this issue May 14, 2019 · 11 comments
Closed

Error handling in tests #771

leoxs22 opened this issue May 14, 2019 · 11 comments
Labels
bug 🐛 Issues that report a problem or error in the code.

Comments

@leoxs22
Copy link
Contributor

leoxs22 commented May 14, 2019

Hello!

I found a really serious bug on the test runner, when there is an exception in some test.

The effect is really hard to detect.

I have this stacktrace:

17:18:32.856 [main] ERROR io.kotlintest.runner.jvm.TestEngine - Error during test engine run
java.lang.reflect.InvocationTargetException: null
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) ~[na:1.8.0_144]
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) ~[na:1.8.0_144]
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) ~[na:1.8.0_144]
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423) ~[na:1.8.0_144]
	at kotlin.reflect.jvm.internal.FunctionCaller$Constructor.call(FunctionCaller.kt:66) ~[kotlin-reflect-1.2.51.jar:1.2.51-release-125 (1.2.51)]
	at kotlin.reflect.jvm.internal.KCallableImpl.call(KCallableImpl.kt:107) ~[kotlin-reflect-1.2.51.jar:1.2.51-release-125 (1.2.51)]
	at kotlin.reflect.jvm.internal.KCallableImpl.callDefaultMethod(KCallableImpl.kt:149) ~[kotlin-reflect-1.2.51.jar:1.2.51-release-125 (1.2.51)]
	at kotlin.reflect.jvm.internal.KCallableImpl.callBy(KCallableImpl.kt:111) ~[kotlin-reflect-1.2.51.jar:1.2.51-release-125 (1.2.51)]
	at kotlin.reflect.full.KClasses.createInstance(KClasses.kt:283) ~[kotlin-reflect-1.2.51.jar:1.2.51-release-125 (1.2.51)]
	at io.kotlintest.runner.jvm.JvmKt.instantiateSpec(jvm.kt:15) ~[kotlintest-runner-jvm-3.3.2.jar:na]
	at io.kotlintest.runner.jvm.TestEngine.createSpec(TestEngine.kt:112) ~[kotlintest-runner-jvm-3.3.2.jar:na]
	at io.kotlintest.runner.jvm.TestEngine.access$createSpec(TestEngine.kt:15) ~[kotlintest-runner-jvm-3.3.2.jar:na]
	at io.kotlintest.runner.jvm.TestEngine$submitSpec$1.run(TestEngine.kt:91) ~[kotlintest-runner-jvm-3.3.2.jar:na]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) ~[na:1.8.0_144]
	at java.util.concurrent.FutureTask.run(FutureTask.java:266) ~[na:1.8.0_144]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) ~[na:1.8.0_144]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) ~[na:1.8.0_144]
	at java.lang.Thread.run(Thread.java:748) ~[na:1.8.0_144]
Caused by: java.lang.NullPointerException: null

The problem is that this error is affecting other tests.

I think that kotlintest is ordering the tests to run alphabetically by name, so, when the engine throws this exception, it doesn't continue with any other test, and it doesn't inform that it failed in any way.

image

It just says that all the test runned fine, and the only way to notice the error, is by noticing that the number of total tests is not what it should be.

So, I think that there are two problems here:

1- If there is an exception in a test, it shouldn't break everything
2- If for any reason the engine fails with an exception, it should be informed in the interface, and it should mark the tests as failed, showing it in maven or gradle.

The bug it's really easy to reproduce, just find the first test that should run alphabetically (or add an AAA as prefix in any test) and put a throw NullPointerException()

In that case, it won't run any test, and it will just say that no test were found.
If you do that for the second test, it will run the first one, and don't say anything about the rest.

The worst about this problem, is that it's really hard to notice that something is failing, so you could have a lot of errors in production believing that all your tests are ok.

@LeoColman
Copy link
Member

Does it fail this way when you run gradlew test?

Or does it explode the exception? Perhaps this is a ide only bug?

@leoxs22
Copy link
Contributor Author

leoxs22 commented May 14, 2019

Does it fail this way when you run gradlew test?

Or does it explode the exception? Perhaps this is a ide only bug?

I'm using maven, not gradle, so I can't confirm if it works there.
In maven, at least, the result it's the same executing mvn test, it says everything it's okay, without running all the tests

@leoxs22
Copy link
Contributor Author

leoxs22 commented May 15, 2019

Adding to this error.

It only happens when the exception is produced outside a test case, in the "init" of the class.

For example, this code can reproduce the error:

class AAAMyTest : WordSpec({

  throw NullPointerException()

  "example" should {
    "run" {
      1 shouldBe 1
    }
  }

})

while this run fine

class AAAMyTest : WordSpec({

  "example" should {
    throw NullPointerException()
    "run" {
      1 shouldBe 1
    }
  }

})

(of course, it doesn't detect that there is a "run" test case, but at least it marks the "example" as failed)

So, the problem occurs when the engine fails to initialize the test class

@leoxs22
Copy link
Contributor Author

leoxs22 commented May 15, 2019

I've tried the same in Junit, coding in Java, and it handles it well.
Adding the nullpointer in the constructor makes it mark all the test cases as failed with a null pointer exception.
I think we can't do something like that, since the tests are written inside the initialization of the class, and by having an exception before, it never reaches the definition of the test, but at least we should be able to mark the full class as failed, and let it continue with the rest of the tests classes.

@LeoColman LeoColman added the bug 🐛 Issues that report a problem or error in the code. label May 15, 2019
@LeoColman
Copy link
Member

I see.

So what's happening is that if the class is throwing an error before the test starts (for instance, before any test case or while inside beforeTest), it doesn't fail as expected.

Does this describe it well enough?

@leoxs22
Copy link
Contributor Author

leoxs22 commented May 15, 2019

I see.

So what's happening is that if the class is throwing an error before the test starts (for instance, before any test case or while inside beforeTest), it doesn't fail as expected.

Does this describe it well enough?

Yep, perfect, it fails to instantiate the test class, and that makes the test engine to fail without informing it correctly

@LeoColman
Copy link
Member

Perfect report, leo!

Thanks for the in-depth explanation. I'll get to understand this right away!

LeoColman added a commit that referenced this issue May 28, 2019
The current way we're using coroutines will make the exceptions be thrown away and ignored. This commit will make the exceptions propagate to the parent, as we were expecting.

This will fix a behavior in which `beforeTest` / `afterTest` wouldn't fail a test when they throw an exception, but instead silently ignore a test.

Fixes #771
@LeoColman LeoColman mentioned this issue Jun 3, 2019
LeoColman added a commit that referenced this issue Jun 4, 2019
The problem with this issue was that any exception that happened in either `BeforeTest` or `AfterTest` was being silently ignored due to Coroutine Launch instead of `async`.

This commit fixes that error, and also modifies listener execution to notify `JUnit` and our executor to be able to identify these errors and show that in the interface.

Part of #771
sksamuel added a commit that referenced this issue Jun 19, 2019
…init block, the beforeSpec method or the afterSpec method #771
sksamuel added a commit that referenced this issue Jun 19, 2019
…init block, the beforeSpec method or the afterSpec method #771
sksamuel added a commit that referenced this issue Jun 22, 2019
#840)

* Fixed test engine reporting when there is an exception in either the init block, the beforeSpec method or the afterSpec method #771

* Updated mocha console #787
sksamuel added a commit that referenced this issue Jun 29, 2019
#840)

* Fixed test engine reporting when there is an exception in either the init block, the beforeSpec method or the afterSpec method #771

* Updated mocha console #787
@leoxs22
Copy link
Contributor Author

leoxs22 commented Jul 1, 2019

Hello!

I've tried the last release (3.3.3) with the changes and it seems to work great! It's throwing the exception and setting it as a failure.

Thanks a lot for your help! I wouldn't be able to solve it myself 😅

The only thing I'm not sure is that it stops running the tests when the exception is found. It should be this way or it should continue with other test classes?

(tagging the PRs that solved the problem)
#865
#840

@sksamuel
Copy link
Member

sksamuel commented Jul 1, 2019 via email

@leoxs22
Copy link
Contributor Author

leoxs22 commented Jul 3, 2019

I don't think it should stop the tests. You're saying it is ?

Yep, doing the same trick (adding "AAA" at the start of the failing class name), it runs only that class, ignoring anything else.

In the first exception, it stops analyzing.

@LeoColman
Copy link
Member

Can we close this issue as solved?

Perhaps we can create a new one to discuss this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Issues that report a problem or error in the code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants