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

Test Case Config does not behave as I expected #476

Closed
Jager567 opened this issue Nov 20, 2018 · 14 comments
Closed

Test Case Config does not behave as I expected #476

Jager567 opened this issue Nov 20, 2018 · 14 comments
Assignees
Labels
bug 🐛 Issues that report a problem or error in the code.
Milestone

Comments

@Jager567
Copy link

The documentation states that I can add a timeout to a test. It states that it is "Useful for code that is non-deterministic and might not finish". This leads me to believe that this setting will stop the test execution after the given duration has passed. However, when I accidentally executed a test that called a method with an infinite loop, it didn't stop the execution after the timeout had passed. Is this a documentation issue, a fault on my side or a bug?

After some testing, I found out that this fails, but only after the full test has finished:

class FiniteLoopString: StringSpec() {
    "has a long finite loop test".config(timeout = 100.milliseconds) {
        val startTime = currentTimeMillis()
        while (currentTimeMillis() < startTime + 1000) {
            "this" shouldNotBe "that"
        }
    }
}

while the following test just keeps running forever:

class InfiniteLoopString: StringSpec() {
    "has an infinite loop test".config(timeout = 100.milliseconds) {
        while (true) {
            "this" shouldNotBe "that"
        }
    }
}

During my testing, I also found that this test succeeds, even though it takes longer than the timeout:

class SuccessfulTest: WordSpec() {
    init {
        "String.length" should {
            "return the length of the string".config(timeout = 2.seconds) {
                val startTime = currentTimeMillis()
                while (currentTimeMillis() < startTime + 10000) {
                    "this" shouldNotBe "that"
                }
            }
        }
    }
}
@sksamuel
Copy link
Member

Definitely a bug. The timeout should kick in for any test that takes longer than the timeout.

@sksamuel
Copy link
Member

Which version of kotlintest ?

@sksamuel sksamuel added the bug 🐛 Issues that report a problem or error in the code. label Nov 20, 2018
@Jager567
Copy link
Author

I tested this with version 3.1.10.

@sksamuel sksamuel added this to the 3.2 milestone Dec 3, 2018
@LeoColman LeoColman self-assigned this Dec 30, 2018
@LeoColman
Copy link
Member

I'll grab this if you're not working on it, @sksamuel .

@sksamuel sksamuel assigned sksamuel and unassigned LeoColman Jan 2, 2019
@sksamuel
Copy link
Member

sksamuel commented Jan 2, 2019

I'll fix this as part of #518 as it will need to work differently.

sksamuel added a commit that referenced this issue Jan 2, 2019
@sksamuel
Copy link
Member

sksamuel commented Jan 2, 2019

Added. Review as part of #518

sksamuel added a commit that referenced this issue Jan 5, 2019
#518)

* all spec DSLs now provide coroutine scopes for suspendable functions; Reimplemented spec runners using coroutines; breaking changes to extension function signatures to add suspend keyword #386

* Reduced logging

* Reduced logging

* Fixed test case timeouts #476

* Update kotlintest-core/src/main/kotlin/io/kotlintest/TestContext.kt

Co-Authored-By: sksamuel <sam@sksamuel.com>

* Addressing review comments

* Reducing logging

* Reducing logging

* GlobalScope -> this

* Tried to improve flakey test

* Fixes tests and coroutines execution

* Updated failed tests to display their exception

* Changed thread-measure strategy

When adding the thread using a suspend-context, the action was being completed too fast, and new threads would never be able to be used. This lead to a bug when using AppVeyor to build the app, as it would never use the 6th thread, no matter how many invocations were configured
@sksamuel
Copy link
Member

sksamuel commented Jan 9, 2019

Please try 3.2.0-RC1 as this should be fixed now. @Jager567

@Jager567
Copy link
Author

Jager567 commented Jan 9, 2019

It seems that test cases now fail correctly, but they still aren't aborted when they haven't finished executing within the timeout period. This means that

class SuccessfulTest: WordSpec() {
	init {
		"String.length" should {
			"return the length of the string".config(timeout = 2.seconds) {
				val startTime = currentTimeMillis()
				while (currentTimeMillis() < startTime + 10000) {
					"this" shouldNotBe "that"
				}
			}
		}
	}
}

now correctly fails, but only after having executed for 10 seconds, while a test with an infinite loop will never finish.

@sksamuel
Copy link
Member

sksamuel commented Jan 9, 2019

A normal test should abort if it's interruptable.

The problem is how do you interrupt a while loop.
You can't.

I can inject a thread interrupt.
I can cancel a coroutine.

But if you want to use an unbounded while loop then you're on your own.

@Jager567
Copy link
Author

Jager567 commented Jan 9, 2019

Because the documentation states that adding a timeout is useful for code that might not finish, I expected that adding a timeout to my test would create something similar to JUnit5 assertTimeoutPreemptively.

That JUnit function does exactly what you claim to be impossible: when it receives an infinite while loop, it will stop the execution when the specified time has passed. It does so by running the test in a new thread. When the timeout has passed, it stops the thread regardless of its state.

I understand that the timeout kotlintest provides is not implemented in the same way. What I do want to suggest is that something is added to the documentation stating that timeouts can only interrupt things that are interruptable.

@sksamuel
Copy link
Member

sksamuel commented Jan 9, 2019

Ok, if junit is doing it, then it's probably using Thread.stop, which I can do as well, but it's been deprecated since Java 1.1. That will actually "kill" a thread in the way I said was impossible. I should have said "impossible without hacky solutions" :)

I suppose we could do the same in theory. Let me look into their implementation.

@sksamuel
Copy link
Member

sksamuel commented Jan 9, 2019

And you're right, at the very least the docs should be clear.

@sksamuel
Copy link
Member

sksamuel commented Jan 9, 2019

So JUnit's implementation is surprising to me.

They just wait xx seconds, and if you're not finished, they'll interrupt your thread, and then carry on, reporting it as failed. So your test report is all hunky dory.

Your thread won't actually be killed though, and it'll just continue in the background until the JVM shuts down. I suppose they are either naive, or assume that the JVM will terminate after the tests complete.

I'm not sure which is better.

@Kerooker thoughts?

@sksamuel
Copy link
Member

In 3.2.1 the behavior is now the same as JUnit.

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

No branches or pull requests

3 participants