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

Add missing assertion params to withCode #7641

Merged
merged 7 commits into from
Dec 28, 2022

Conversation

matthewjones372
Copy link
Contributor

@matthewjones372 matthewjones372 commented Dec 16, 2022

Adds additional RenderParams to the with code method for rendering test assertions. Forward ported what was needed from the Render trait in the zio 1 branch.
Closes #7602

@CLAassistant
Copy link

CLAassistant commented Dec 16, 2022

CLA assistant check
All committers have signed the CLA.

@matthewjones372 matthewjones372 changed the title Improve with code Add missing assertion params in withCode Dec 16, 2022
@regiskuckaertz
Copy link
Member

@swoogles hello 👋 this is my colleague doing his first ZIO pr 🕺 We found the PrettyPrint function that was quite handy and thought it would be better to reuse that rather than forward port the Render machinery.

test/shared/src/main/scala/zio/test/Assertion.scala Outdated Show resolved Hide resolved
test/shared/src/main/scala/zio/test/Assertion.scala Outdated Show resolved Hide resolved
test/shared/src/main/scala/zio/test/Assertion.scala Outdated Show resolved Hide resolved
test/shared/src/main/scala/zio/test/Assertion.scala Outdated Show resolved Hide resolved
test/shared/src/main/scala/zio/test/Assertion.scala Outdated Show resolved Hide resolved
test/shared/src/main/scala/zio/test/Assertion.scala Outdated Show resolved Hide resolved
test/shared/src/main/scala/zio/test/Assertion.scala Outdated Show resolved Hide resolved
test/shared/src/main/scala/zio/test/Assertion.scala Outdated Show resolved Hide resolved
test/shared/src/main/scala/zio/test/TestArrow.scala Outdated Show resolved Hide resolved
@matthewjones372
Copy link
Contributor Author

Before continuing, I'm not sure if this is the right path. Would be interested in hearing some feedback! thanks

@adamgfraser
Copy link
Contributor

@matthewjones372 I think it is fine to use withCode here. My main concern is that the proposed type signature of withCode is very weakly typed:

def withCode(code: String, params: Any*): TestArrow[A, B] =
  ???

It seems like the contract is that each element of params will be rendered as a String and then they will be rendered as code($param1, $param2, ...). One thing that might make this more clear is just accepting a variable arguments list of String, though maybe you were trying to avoid the need to call toString every time you call withCode. The other thing we're not able to express here is any assertions that have a more complex pattern of rendering, for example curried arguments. That is a little bit of what that Render data type did before.

So I think you could use withCode but it might be helpful if it accepted a more structured representation of the argument and its parameters that was an algebraic data type that could then get rendered as a String.

@swoogles
Copy link
Contributor

@adamgfraser So are you envisioning something like:

def withCode(code: String, render: Assertion.Render): TestArrow[A, B] = ???

?

@adamgfraser
Copy link
Contributor

Or even:

def withCode(render: Assertion.Render): TestArrow[A, B] = ???

May not be able to do that for compatibility so it could be:

def withCode(code: String, arguments: Arguments): TestArrow[A, B] = ???

@matthewjones372 matthewjones372 marked this pull request as ready for review December 20, 2022 15:13
@matthewjones372 matthewjones372 marked this pull request as draft December 20, 2022 15:26
@adamgfraser adamgfraser marked this pull request as ready for review December 20, 2022 17:29
Copy link
Member

@regiskuckaertz regiskuckaertz left a comment

Choose a reason for hiding this comment

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

Looks like this is almost there, what do you think @adamgfraser ?

test/shared/src/main/scala/zio/test/Assertion.scala Outdated Show resolved Hide resolved
test/shared/src/main/scala/zio/test/Assertion.scala Outdated Show resolved Hide resolved
test/shared/src/main/scala/zio/test/Assertion.scala Outdated Show resolved Hide resolved
@matthewjones372 matthewjones372 changed the title Add missing assertion params in withCode Add missing assertion params to withCode Dec 21, 2022
@matthewjones372 matthewjones372 force-pushed the improve_with_code branch 3 times, most recently from 4f1efe6 to 6f07119 Compare December 21, 2022 23:55
@regiskuckaertz
Copy link
Member

Looks like there are some flaky tests, but this one I believe is a result of this PR, but the expectation should be amended to reflect the new ouput IMO:

"""##teamcity[testStarted name='labeled failures' locationHint='file:///home/runner/work/zio/zio/test-tests/shared/src/test/scala/zio/test/IntellijRendererSpec.scala:36' captureStandardOutput='true']
      ##teamcity[testFailed name='labeled failures' message='Assertion failed:' details='  ✗ 0 was not equal to 1
        third
        c did not satisfy isSome(equalTo(1)).label(\"third\")
        isSome(equalTo(1)) = 0
        c = Some(0)
        at /home/runner/work/zio/zio/test-tests/shared/src/test/scala/zio/test/IntellijRendererSpec.scala:36 
      ']
      """ did not contain """##teamcity[testFailed name='labeled failures' message='Assertion failed:' details='  ✗ 0 was not equal to 1
        third
        c did not satisfy isSome(equalTo(1)).label(\"third\")
        isSome = 0
        c = Some(0)
        at /home/runner/work/zio/zio/test-tests/shared/src/test/scala/zio/test/IntellijRendererSpec.scala:36 
      ']
      """

Copy link
Contributor

@adamgfraser adamgfraser left a comment

Choose a reason for hiding this comment

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

@matthewjones372 Thank you for your work on this and congratulations on your first contribution to ZIO!

@adamgfraser adamgfraser merged commit e4afc37 into zio:series/2.x Dec 28, 2022
@matthewjones372
Copy link
Contributor Author

@matthewjones372 Thank you for your work on this and congratulations on your first contribution to ZIO!

thanks!

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.

Add missing assertion rendering API
5 participants