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

Replace expecty with clue #64

Merged
merged 2 commits into from
Oct 9, 2024

Conversation

zainab-ali
Copy link
Contributor

@zainab-ali zainab-ali commented Sep 16, 2024

This PR replaces expecty with an munit-like clue function.

It uses macros to generate clues. Unlike munit, clues are stored locally to the expect call. This avoids the need for a global lock, and means tests can run concurrently, and add clues, without relying on any extra concurrency primitives.

It assumes that the code within an expect call is synchronous.

The behaviour is identical for Scala 2 and 3, but the macros are implemented differently.

Note that the signatures for expect aren't identical for Scala 2 and 3. The Scala 3 expect functions use context functions.

Behaviour

A user can write:

val x = 1
val y = 2
expect(clue(x) == clue(y))

and will see the following on test failure:

Clues {
  x: Int = 1
  y: Int = 2
}

They can also nest calls to clue:

val x = 1
val y = 2
expect(clue(List(clue(x))) == clue(List(clue(y))))

Scala 2 implementation

The Scala 2 implementation has a couple of macros:

  • The ClueMacro generates a clue for a value expression. This is identical to munit's clue macro.
  • The ExpectMacro constructs a local collection of Clues. Calls to the clue function are rewritten to call the addClue function of the local collection.

Scala 3 implementation

In Scala 3, we can use context functions to provide the Clues collection. It still has a couple of macros:

  • The ClueMacro generates a clue for a value expression using simpler logic for obtaining the source code and type.
  • The ExpectMacro gets the source location to ensure that the tracing behaviour of expect is the same as it was previously. Calls to clue don't need to be rewritten.

@zainab-ali zainab-ali marked this pull request as draft September 16, 2024 13:24
@zainab-ali zainab-ali force-pushed the munit-clue-integration branch 3 times, most recently from 6f24200 to 1bda9bb Compare September 17, 2024 14:16
@zainab-ali zainab-ali force-pushed the munit-clue-integration branch from 1bda9bb to e6bc0a6 Compare September 17, 2024 14:23
@zainab-ali zainab-ali marked this pull request as ready for review September 17, 2024 14:27
@zainab-ali zainab-ali changed the title WIP: Basic clue implementation. Replace expecty with clue Sep 17, 2024
@zainab-ali zainab-ali mentioned this pull request Sep 19, 2024
16 tasks
import c.universe._

def fromContext: Tree = {
def fromContext: c.Tree = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

"This function can only be used within `expect` or `assert`.")
final def clue[A](@unused a: Clue[A]): A = {
// This function is removed as part of the `expect` macro expansion.
throw new Error("compileTimeOnly annotation not respected! This is likely to be a bug in weaver-test. Report it at https://github.com/typelevel/weaver-test/issues/new")
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

override def transform(input: Tree): Tree = input match {
case c.universe.Apply(fun, List(clueValue))
if fun.symbol == clueMethodSymbol =>
val transformedClueValue = super.transform(clueValue)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious about this transformation here. Can you explain ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course!

For context, the user writes clue(x) where clue refers to ClueHelpers.clue. There's an implicit conversion from x to a Clue. Explicitly, this is written as clue(Clue.generateClue(x)) .

The transformation replaces ClueHelpers.clue with addClue on a clues collection. In other words, it replaces:
clue(Clue.generateClue(x)) with clues.addClue(Clue.generateClue(x)).

The clues collection is generated as part of the overall expect macro. For example:

expect(clue(List(x, clue(y))))

becomes

expect {
  val clues = new Clues()
  clues.addClue(List(x, clues.addClue(y)))
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh gotcha, hadn't thought of nested clues, but it makes sense 👍

@Baccata
Copy link
Collaborator

Baccata commented Oct 1, 2024

This is great ! Thank you so much for taking that on !🥇

I just have a minor question, but I'm happy with the code, tests, and documentation.

@Baccata Baccata merged commit 8aeb1bc into typelevel:main Oct 9, 2024
13 checks passed
@zainab-ali zainab-ali deleted the munit-clue-integration branch October 9, 2024 11:42
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