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

optimize test of withNull #41

Closed
wants to merge 1 commit into from

Conversation

TysonMN
Copy link
Member

@TysonMN TysonMN commented Nov 11, 2020

In hedgehogqa/fsharp-hedgehog#154, @cmeeren suggested that the test of withNull was inefficient. Here are the runtimes of this test reported by the Test Explorer in Visual Studio.

  • When running just this test: ~250 ms
  • When running all tests: 7-17 ms

In fact, this test is among the fastest tests in the test suite.

This PR rewrites this test to do exactly what is desired by the test. It does not generate unnecessary values and it does not do any retests with shrunk values. It is easy to verify that this test passes for the right reason because removing the call to withNull causes the test to fail. Here are the runtimes reported by the Test Explorer in Visual Studio.

  • When running just this test: 166-141 ms
  • When running all tests: 6 ms

This rough analysis shows that this test that was already among the fastest got a little bit faster.

My rewrite of this test contains more code. It also contains more lines although some of them could be combined. Another disadvantage is that Hedgehog.Random might disappear one day soon. This will change the API, but I think the functionality will remain.

On the positive side, I think this implementation is more idiomatic.

@cmeeren
Copy link
Collaborator

cmeeren commented Nov 11, 2020

Thanks. I'm wary of merging this since 1) there's no important performance benefit, 2) it's more complex (requires knowledge about more Hedgehog stuff that at least I don't know anything about), and 3) you mention that Random might disappear. So I suggest we close this without merging. What do you say?

@TysonMN
Copy link
Member Author

TysonMN commented Nov 11, 2020

What do you say?

That's fine. At least it is a specific proof of concept for an alternative implementation.

@TysonMN TysonMN closed this Nov 11, 2020
@TysonMN TysonMN deleted the loop_withNull_test branch November 12, 2020 13:52
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.

2 participants