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

Only render subject description on failure #292

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dump247
Copy link
Contributor

@dump247 dump247 commented Jun 1, 2023

Use a lambda to lazily render the subject description only when the test fails. This significantly speeds up the test execution. In particular, this can really help property based tests where thousands of tests may be executed.

Constructing an exception is relatively fast and it captures the stack trace. Converting the stack trace from the internal representation to Java StackTraceElements is very costly (i.e. calling ex.getStackTrace). I had to recreate getCallerInfo from FilePeek to accept an exception rather than creating one internally. I submitted a PR to filepeek that adds a getCallerInfo that accepts an exception.

This super contrived test goes from 10.6 seconds to 1.1s (on my machine).

@Test
fun `get perf`() {
  class Person(val id: Int)
  val range = Int.MIN_VALUE..Int.MAX_VALUE

  repeat(10_000) {
    expectThat(Person(Random.nextInt())) {
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
      get { id } isIn range
    }
  }
}

Use a lambda to lazily render the subject description only when the test fails.
This significantly speeds up the test execution.

private val sourceRoots: List<String> = listOf("src${FS}test${FS}kotlin", "src${FS}test${FS}java")

private fun filepeek.FilePeek.specialGetCallerInfo(ex: Throwable): FileInfo {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this PR for filepeek

christophsturm/filepeek#14

LambdaBody("get", line).body.trim()
} catch (e: Exception) {
"%s"
var lambda = DESCRIBE_CACHE[javaClass]
Copy link
Contributor Author

@dump247 dump247 Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second commit is some further experimentation in caching the describe string calculation. I don't know if it is guaranteed behavior (I can't find any documentation), but the class created for each lambdas is unique for the location of the lambda. In my get perf test, the class names are something like "get perf"$1$1, "get perf"$1$2, and so on.

Adding a cache moves the example test from 1.1s to 200ms.

I imagine this will have less real effect than my example test shows because the cache key is per lambda. So different lambdas will have different cache keys even if the lambdas are defined identically. This realistically only helps within a single test.

@dump247
Copy link
Contributor Author

dump247 commented Jun 2, 2023

@robfletcher Just wondering how you feel about the direction. I think the first commit is a pretty modest change with a significant effect. The second commit is more questionable. I am not sure how to manage cache eviction. Let me know your thoughts and I will put some time into writing up some unit tests.

I really enjoy strikt. Thanks for making it available.

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

Successfully merging this pull request may close these issues.

1 participant