Add support for Hedgehog property testing #78

mpilquist · 2020-03-28T15:21:39Z

Opening this draft PR for feedback on overall API. Besides review comments, this also needs the following updates before merging:

Hedgehog needs to publish for Scala Native
Hedgehog PR for effectful properties needs to be merged (Future result hedgehogqa/scala-hedgehog#147)
Somehow skip Scala.js 0.6 publishing of hedghog project

olafurpg

Thank you for this contribution! I'm excited to try this out myself. I have never enjoyed using ScalaCheck so much and I have heard great things about the Haskell implementation of Hedgehog.

Please let us know if there's anything we can do to unblock this PR

olafurpg · 2020-03-31T20:17:21Z

munit-hedgehog/shared/src/main/scala/munit/HedgehogSuite.scala

+
+  def property(
+      name: String,
+      config: PropertyConfig


To reduce the number of method overloads, would it make sense to pass the property config via tags instead?

property("name".tag(customPropertyConfig))

I looked in to that but note doing so would require the ability to convert property config to/from String, unless I'm misunderstanding.

BTW, I used Conversion to avoid the need for importing language.implicitConversions when working with Dotty. Maybe we should do the same for Conversion[String, TestOptions]?

It's possible to create a tag that contains custom values like this

case class HedgehogConfig(config: PropertyConfig) extends Tag("HedgehogConfig")

See https://scalameta.org/munit/docs/tests.html#customize-evaluation-of-tests-with-tags

I didn't know about the Conversion trick to avoid the import. That's handy. However, I think it's good to keep the name: String method overload to make the sources more approachable for Scala beginners

Ah, cool, I'll update the overloads.

Re: Conversion, I'm getting warnings with munit 0.7.1 when calling test("name".ignore). I was wondering if we could use Conversion to avoid such warnings. I'll open a separate issue on that.

olafurpg · 2020-03-31T20:22:13Z

build.sbt

+    sharedSettings,
+    crossScalaVersions := List(scala213, scala212, scala211, dotty),
+    resolvers += "bintray-scala-hedgehog".at(
+      "https://dl.bintray.com/hedgehogqa/scala-hedgehog"


It would be nice to have a hedgehog release on Maven Central.

Agreed, there's an open issue for it and lead maintainer has been working it but not sure how close it is to being merged.

gabro · 2020-04-01T08:39:45Z

munit-hedgehog/shared/src/main/scala/munit/HedgehogSuite.scala

+   * Checks the supplied `Property[Result]`, throwing a `HedgehogFailException`
+   * if the property was falsified.
+   */
+  def check(


would it make sense to make the check methods private?

I made it public b/c in scodec-bits, I had some cases where I wanted to call check multiple times in a single test. That's not really necessary though so I'm happy with this being private.

gabro · 2020-04-01T08:46:27Z

munit-hedgehog/shared/src/main/scala/munit/HedgehogSuite.scala

+  lazy val propertyConfig: PropertyConfig = PropertyConfig.default
+  lazy val propertySeed: Long = System.currentTimeMillis()


Not a huge deal, but I would like to standardize these with respect to the scalacheck module. A few differences I can spot:

we use protected in ScalaCheckSuite (maybe redundant, but I think it makes sense)

we use def for the config, to allow overriding with anything (def, val, lazy val) instead of forcing to use val in the override

we prefix the variables using scalaCheck as in scalaCheckTestParameters. Since these modules are defined as trait I wanted to maintain some kind of namespace to avoid potential clashes when we add more modules that can be mixed and matched.

I'm fine to standardize either way, as long as it's consistent.

What do you think @mpilquist?

👍 on protected and prefixing names. Re: def versus val, I used lazy val in order to guarantee the same seed is used in all tests in a suite. I'm okay with using a def instead though. No strong opinion.

def versus val, I used lazy val in order to guarantee the same seed is used in all tests in a suite

👍 makes sense if that's the intended semantics. Does the same reasoning apply to the config?

gabro · 2020-04-01T08:48:10Z

munit-hedgehog/shared/src/main/scala/munit/HedgehogSuite.scala

+  lazy val propertyConfig: PropertyConfig = PropertyConfig.default
+  lazy val propertySeed: Long = System.currentTimeMillis()


Also, does it make sense to memoize the seed? IIUC in ScalaCheck the seed is generated fresh for each prop

Minor, but would it make sense to use

Suggested change

lazy val propertyConfig: PropertyConfig = PropertyConfig.default

lazy val propertySeed: Long = System.currentTimeMillis()

lazy val propertySeed: Seed = Seed.fromTime()

instead?

See https://github.com/hedgehogqa/scala-hedgehog/blob/master/core/shared/src/main/scala/hedgehog/core/Seed.scala#L48-L49

Yeah I'd like to use Seed but need to figure out a way to convert an arbitrary seed to/from a string for reporting.

I ended up using String myself in ScalaCheckSuite, since it's then easier for the user to set it when trying to reproduce a failure.

See the updated docs at https://scalameta.org/munit/docs/integrations/scalacheck.html#reproducing-a-property-failure

gabro · 2020-04-01T08:50:47Z

tests/shared/src/main/scala/munit/ScalaCheckFrameworkSuite.scala

@@ -7,7 +7,7 @@ class ScalaCheckFrameworkSuite extends ScalaCheckSuite {

  // NOTE(gabro): this is needed for making the test output stable for the failed test below.
  // It also serves as a test for overriding these parameters.
-  override def scalaCheckTestParameters =
+  override def scalaCheckTestParameters: org.scalacheck.Test.Parameters =


was this required by Scalafix? I must have missed it

anyway, if you rebase on master that's gone now

olafurpg · 2020-04-06T14:37:51Z

tests/shared/src/main/scala/munit/HedgehogFrameworkSuite.scala

@@ -42,8 +42,7 @@ class HedgehogFrameworkSuite extends HedgehogSuite {
  }

  property(
-    "custom config",
-    config = propertyConfig.copy(testLimit = SuccessCount(1000))
+    "custom config".tag(HedgehogConfig(hedgehogPropertyConfig.copy(testLimit = SuccessCount(1000))))


This is quite a mouthful, I wonder if we want to add custom tags for the most common configuration options? Just brainstorming, it would be nice if the user could do something like this instead 🤔

Suggested change

"custom config".tag(HedgehogConfig(hedgehogPropertyConfig.copy(testLimit = SuccessCount(1000))))

"custom config".tag(TestLimit(1000))

If we go down this route I would still namespace the tags

Suggested change

"custom config".tag(HedgehogConfig(hedgehogPropertyConfig.copy(testLimit = SuccessCount(1000))))

"custom config".tag(HedgehogTestLimit(1000))

This is totally optional BTW, just a suggestion. I'm happy to merge this PR as is. I believe we can update this PR to skip projects where the cross-build is missing

I agree it's too verbose as-is. I'll update like you suggested. I think I'll add support for passing a seed as a tag as well.

I can also remove the propertyF stuff for now (until Hedgehog merges support for that).

One note about tags - figuring out the supported tags is not discoverable via auto-completion. We could address that via extension methods - what do you think?

olafurpg · 2021-10-16T08:28:26Z

Closing this PR since it's been inactive for a while. Don't hesitate to reopen if you want to pick this up again!

mpilquist added 6 commits March 28, 2020 11:19

Add support for Hedgehog property testing

48026cb

Fix 2.11 and 0.23 builds

9537fa6

Format source

5a50759

Format sbt

b17edc2

Fix Scala.js dependency and fix JVM test

11ab263

Fix tests by removing result stack trace

2c7e56f

olafurpg reviewed Mar 31, 2020

View reviewed changes

gabro reviewed Apr 1, 2020

View reviewed changes

mpilquist added 2 commits April 3, 2020 09:17

Merge branch 'master' into topic/hedgehog

5609ed0

Review comments

13d17eb

olafurpg reviewed Apr 6, 2020

View reviewed changes

Base automatically changed from master to main January 20, 2021 08:13

olafurpg closed this Oct 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Hedgehog property testing #78

Add support for Hedgehog property testing #78

mpilquist commented Mar 28, 2020 •

edited

Loading

olafurpg left a comment

olafurpg Mar 31, 2020

mpilquist Mar 31, 2020

mpilquist Mar 31, 2020

olafurpg Mar 31, 2020

mpilquist Mar 31, 2020

olafurpg Mar 31, 2020

olafurpg Mar 31, 2020

mpilquist Mar 31, 2020

gabro Apr 1, 2020

mpilquist Apr 1, 2020

gabro Apr 1, 2020

mpilquist Apr 1, 2020

gabro Apr 1, 2020

gabro Apr 1, 2020

gabro Apr 1, 2020

mpilquist Apr 1, 2020

gabro Apr 1, 2020

gabro Apr 1, 2020

gabro Apr 1, 2020

mpilquist Apr 1, 2020

olafurpg Apr 6, 2020

gabro Apr 6, 2020

olafurpg Apr 6, 2020

mpilquist Apr 6, 2020

olafurpg commented Oct 16, 2021

		lazy val propertyConfig: PropertyConfig = PropertyConfig.default
		lazy val propertySeed: Long = System.currentTimeMillis()

	lazy val propertyConfig: PropertyConfig = PropertyConfig.default
	lazy val propertySeed: Long = System.currentTimeMillis()
	lazy val propertySeed: Seed = Seed.fromTime()

	"custom config".tag(HedgehogConfig(hedgehogPropertyConfig.copy(testLimit = SuccessCount(1000))))
	"custom config".tag(TestLimit(1000))

	"custom config".tag(HedgehogConfig(hedgehogPropertyConfig.copy(testLimit = SuccessCount(1000))))
	"custom config".tag(HedgehogTestLimit(1000))

Add support for Hedgehog property testing #78

Add support for Hedgehog property testing #78

Conversation

mpilquist commented Mar 28, 2020 • edited Loading

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg commented Oct 16, 2021

mpilquist commented Mar 28, 2020 •

edited

Loading