-
Notifications
You must be signed in to change notification settings - Fork 337
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 method checkpoint
to trait Checkpoints for easy use of Checkpoint
#2305
Conversation
Hi @ndy2, we require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please access https://www.artima.com/cla/choose-type to sign our Contributor License Agreement. Your effort is highly appreciated. Thank you. |
@cla-bot[bot] check |
The cla-bot has been summoned, and re-checked this pull request! |
Hello, @cheeseng ! |
@ndy2 Sorry for the late response, I'll take a look today and what's going on. Cheers. |
@ndy2 I managed to fix the compile errors: But the unit tests are failing because of wrong line number check, I'll try to look into it further when possible. |
I merged the PR in my fork. |
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 seems like a nice addition. But I would probably call the method withCheckpoint rather than checkpoint. And the signature should be:
def withCheckpoint(f: Checkpoint => Assertion)(implicit pos: source.Position): Assertion
Instead of:
def checkpoint(f: Checkpoint => Unit)(implicit pos: source.Position): Unit
@bvenners Hi. The suggested signature looks good. I agree that But how can we use I found that below usage withCheckpoint { cp =>
cp { x should be < 0 }
cp { y should be > 9 }
} or to write as below which is quite not awesome. below withCheckpoint { cp =>
cp { x should be < 0 }
cp { y should be > 9 }
Succeeded
} Can you explain what do you want to achieve by use |
You're right. The problem is that Checkpoint's apply method returns Unit. It should probably return Assertion, but we did not change that when we added Assertion. Please leave it as Unit in your PR and we can use that for now. Perhaps later we can think about migrating to Assertion. |
I added a method
checkpoint
to trait Checkpoints for easy use ofCheckpoint
.I have some experience of other test frameworks of JVM familiy like Junit/assertj & kotest.
I thought that create Checkpoint & report it is quite verbose and tedious.
Thus, I introduce a checkpoint!
I found that like the trait
Checkpoint
is right place to place this method like other dsl helper traits in scalatest. (Inside
,Matchers
...)Full Usage Example :
Inspired by