-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add TestAspect.checks to allow running aspects within property-based specs #9076
Conversation
💵 To receive payouts, sign up on Algora, link your Github account and connect with Stripe. |
12dadc3
to
ba6f8f6
Compare
* NOTE: default implementation for backward compatibility. Remove in next | ||
* major version. | ||
*/ | ||
def checkSampleAspect: TestAspect.CheckSampleAspect = ZIOAspect.identity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guy is a bit unfortunate as it's possible to forget to forward the aspect when creating proxies like in TestAspect.scala, but it's the most backward-compatible approach I could come up with.
Suggestions welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had this issue myself a few times.
Scala needs an annotation on methods that should always be overwritten by concrete implementations 🥲
9233ad1
to
ad1b1fd
Compare
ad1b1fd
to
842f8c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks great! Just some minor comments below
@@ -56,23 +65,57 @@ object TestConfig { | |||
|
|||
final case class Test(repeats: Int, retries: Int, samples: Int, shrinks: Int) extends TestConfig | |||
|
|||
// NOTE: Additonal class for backward compatibility. Rename to `Test` and remove the old one in next major version. | |||
final case class Test1( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next major version won't likely be for some time. Perhaps rename this to TestV2
add a @deprecated("use TestV2", "2.1.8")
annotation to Test
?
case Spec.TestCase(oldTest, annotations) => | ||
val newTest = makeAspect.mapError(TestFailure.fail).flatMap { aspect => | ||
testConfigWith { oldConfig => | ||
val newConfig = new TestConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use the TestV2
case class here instead. I don't really know why the Test
case class wasn't used here but I can't think of a good reason
val retries = old.retries | ||
val samples = old.samples | ||
val shrinks = old.shrinks | ||
val repeats = n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
val retries = n | ||
val samples = old.samples | ||
val shrinks = old.shrinks | ||
val repeats = old.repeats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
val retries = old.retries | ||
val samples = n | ||
val shrinks = old.shrinks | ||
val repeats = old.repeats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
val retries = old.retries | ||
val samples = old.samples | ||
val shrinks = n | ||
val repeats = old.repeats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
81f0816
to
6c8879a
Compare
6c8879a
to
a4a2787
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the nitpick 😅
It's good to go after this
import zio.System.env | ||
import zio.test.TestAspectAtLeastR | ||
import zio.test.TestAspectAtLeastR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
resolves #9046
/claim #9046