Extract diff module #756

majk-p · 2024-03-26T14:03:12Z

This PR aims to extract diff calculation to a separate module to resolve #745

Since diff had to share a bit of code with the rest of munit, I've also extracted munit-core that provides the minimal common part.

majk-p · 2024-03-26T16:20:47Z

munit/shared/src/main/scala/munit/Assertions.scala

+  private def exceptionHandlerFromAssertions(
+      assertions: Assertions,
+      clues: => Clues
+  ): ComparisonFailExceptionHandler =
+    new ComparisonFailExceptionHandler {
+      def handle(
+          message: String,
+          obtained: String,
+          expected: String,
+          loc: Location
+      ): Nothing = {
+        assertions.failComparison(message, obtained, expected, clues)(loc)
+      }
+    }


I decided to extract this from ComparisonFailExceptionHandler so that the scala-diff package doesn't need to depend on Assertions

majk-p · 2024-03-26T17:02:29Z

@valencik @tgodzik this change breaks mima on purpose due to reorganisation of modules. Please advise what's your preferred strategy to deal with mima check considering 1.0.0 is underway.

tgodzik · 2024-03-27T19:10:08Z

build.sbt

+  .jsConfigure(sharedJSConfigure)
+  .jsSettings(sharedJSSettings)
+
+lazy val munitDiff = crossProject(JSPlatform, JVMPlatform, NativePlatform)


Ideally munitDiff would not depend on anything, it should not be about testing really.

As far as I understand it, both diff and munit need things like Location along with macros that generate it. Do you prefer code duplication over shared core module?

I would not use Location for diff if possible, seems like it's not really core functionality.

I think we can try getting rid of Location, I'll give it a try. What about AnsiColors? Should it be a part of diff module?

I think so, since it might be useful for showing the diff.

tgodzik · 2024-03-27T19:11:08Z

@valencik @tgodzik this change breaks mima on purpose due to reorganisation of modules. Please advise what's your preferred strategy to deal with mima check considering 1.0.0 is underway.

Extracting munitDiff will break mima anyway. It's probably the last chance to do it. We should work on actual release later on. Any objections?

valencik · 2024-03-29T14:08:59Z

@valencik @tgodzik this change breaks mima on purpose due to reorganisation of modules. Please advise what's your preferred strategy to deal with mima check considering 1.0.0 is underway.

I first want to note that the ecosystem is eager for a 1.0 release.
It is not ideal to be on milestones this long.
And we have a split of projects on 0.7.x and 1.0.x milestones throughout the ecosystem.

However, I wanted to at least bump us to the latest Scala 3 LTS before cutting 1.0.
Unfortunately, we're blocked by scala/scala3#19594 not having yet been ported back to a LTS version.

All this to say, I don't have strong objections to the plan here.

zainab-ali · 2024-04-02T15:16:58Z

scala-diff/shared/src/main/scala/munit/diff/Diffs.scala

@@ -1,4 +1,4 @@
-package munit.internal.difflib
+package munit.diff

 import munit.Location



I wonder if assertNoDiff should still be part of this Diffs object? Each testing library does assertions differently.

It might be better to have something like createReportIfDiff and use this in Assertions:

def createReportIfDiff( obtained: String, expected: String, title: String, printObtainedAsStripMargin: Boolean ): Option[String] = { if (obtained.isEmpty && !expected.isEmpty) { val msg = s"""|Obtained empty output! |=> Expected: |$expected""".stripMargin Some(msg) } else { val diff = new Diff(obtained, expected) if (diff.isEmpty) None else { Some(diff.createReport(title, printObtainedAsStripMargin)) } } }

However, I'm not sure how we feel about removing assertNoDiff from here entirely.

I think that's totally fine, asserts can live elsewhere

zainab-ali · 2024-04-16T11:48:26Z

scala-diff/shared/src/main/scala/munit/diff/Clue.scala

@@ -0,0 +1,13 @@
+package munit.diff
+
+class Clue[+T](


I don't think Clue should be a type in the diff library. From what I understand, it's been moved here so that we can give it a custom representation in Printers.

That being said, I haven't thought of alternative approaches.

Me neither, I did this to satisfy the compiler, I also don't have a better idea yet

tgodzik · 2024-04-16T13:13:46Z

munit/shared/src/main/scala/munit/Assertions.scala

@@ -51,10 +54,10 @@ trait Assertions extends MacroCompat.CompileErrorMacro {
      clue: => Any = "diff assertion failed"
  )(implicit loc: Location): Unit = {
    StackTraces.dropInside {
-      Diffs.assertNoDiff(
+      DiffsAssetion.assertNoDiff(


Suggested change

DiffsAssetion.assertNoDiff(

DiffsAssertion.assertNoDiff(

Could you fix this one? It's super minor

tgodzik · 2024-04-16T13:38:58Z

scala-diff/shared/src/main/scala/munit/diff/console/Printers.scala

+          case x: Float  => out.append(x.toString())
+          case x: Double => out.append(x.toString())
+          case x: String => printString(x, out, printer)
+          case x: Clues =>


Could we just make Printer a class and add support for Clues in a child class?

Or maybe a custom class like:

trait CustomPrintable{ def append(out: StringBuilder) }

and make Clue implement it?

tgodzik · 2024-04-16T13:39:43Z

munit/shared/src/main/scala/munit/Clues.scala

-  def empty: Clues = new Clues(List())
-  def fromValue[T](value: T): Clues = new Clues(List(Clue.fromValue(value)))
-}
+// class Clues(val values: List[Clue[_]]) {


TODO remove if we decide to leave Clue in the current position

tgodzik · 2024-04-23T11:49:23Z

scala-diff/shared/src/main/scala/munit/diff/NextPrintable.scala

+package munit.diff
+
+/**
+ * Don't override this ever


Would be great to mention why a bit

No longer relevant, shouldn't be pushed in the first place 😉

tgodzik · 2024-04-23T11:50:16Z

munit/shared/src/main/scala/munit/Assertions.scala

@@ -51,10 +54,10 @@ trait Assertions extends MacroCompat.CompileErrorMacro {
      clue: => Any = "diff assertion failed"
  )(implicit loc: Location): Unit = {
    StackTraces.dropInside {
-      Diffs.assertNoDiff(
+      DiffsAssetion.assertNoDiff(


Could you fix this one? It's super minor

tgodzik · 2024-04-23T12:09:19Z

You might want to rebase the PR to see if it passes.

majk-p · 2024-04-23T14:28:15Z

After brainstorming with @zainab-ali and inspired by this commit I did an overhaul of this change. Most of the Printers functionality remains in munit while only logic for string cleanup is extracted to diff (since it's the only used part). I think it makes things cleaner and we no longer have to deal with disentangling Clues.

tgodzik · 2024-04-23T16:18:27Z

Looks like the compilation is failing, could you take a look?

majk-p · 2024-04-23T16:25:19Z

I just run compile and test locally and everything worked well. I cannot see the CI logs in actions 😞

tgodzik · 2024-04-23T16:34:06Z

build.sbt

@@ -214,7 +214,7 @@ lazy val munit = crossProject(JSPlatform, JVMPlatform, NativePlatform)
  .settings(
    sharedSettings,
    Compile / unmanagedSourceDirectories ++=
-      crossBuildingDirectories("munit", "main").value,
+      crossBuildingDirectories("scala-diff", "main").value,


Is this change correct?

That's right, missed it during cleanup

tgodzik

LGTM from me! Thanks for the great work!

I added 3 more commits:

rename to munit-diff since we have package munit.diff everywhere so it makes much more sense
fix mdoc docs generation
fix mima exceptions

@zainab-ali what do you think?

My next plan is to remove any deprecation we can find and release an RC1

zainab-ali · 2024-04-24T09:52:22Z

Thanks @majk-p and @tgodzik, the PR itself looks great!

Unfortunately the rencent changes to munit include a Native upgrade to 0.5. Weaver has a way to go until moving to 0.5 itself - see the discussion for cats-effect.

I'm brainstorming approaches we could take. Would it be viable to release this on 0.4.x too?

tgodzik · 2024-04-24T10:00:29Z

I can do that manually later on. Hopefully, this time it will work reasonably.

changes because munit-diff is now an independent artifact: scalameta/munit#756 * add dependency for munit-diff * adapt imports

majk-p commented Mar 26, 2024

View reviewed changes

tgodzik reviewed Mar 27, 2024

View reviewed changes

zainab-ali reviewed Apr 2, 2024

View reviewed changes

majk-p force-pushed the extract-diff-module branch from d2a90bd to 3aad768 Compare April 16, 2024 09:15

zainab-ali reviewed Apr 16, 2024

View reviewed changes

zainab-ali mentioned this pull request Apr 16, 2024

Use munit diff logic typelevel/weaver-test#25

Merged

2 tasks

tgodzik reviewed Apr 16, 2024

View reviewed changes

tgodzik reviewed Apr 23, 2024

View reviewed changes

majk-p force-pushed the extract-diff-module branch from 374926c to e728d92 Compare April 23, 2024 13:56

Extract munit diff module

f0a004c

majk-p force-pushed the extract-diff-module branch from e728d92 to f0a004c Compare April 23, 2024 14:25

DiffsAssertion -> Diffs

a11ef11

tgodzik reviewed Apr 23, 2024

View reviewed changes

majk-p and others added 5 commits April 23, 2024 18:38

fix cross build

c8feb48

scalafix

19ca9f4

refactor: Rename to munit-diff instead of scala-diff

cbbbd09

chore: Fix mdoc compilation

c5ce308

chore: Ignore mima filters and disable for now for munit-diff

25ce89f

tgodzik approved these changes Apr 24, 2024

View reviewed changes

tgodzik requested a review from zainab-ali April 24, 2024 08:26

zainab-ali approved these changes Apr 24, 2024

View reviewed changes

tgodzik merged commit a66abd5 into scalameta:main Apr 24, 2024
9 checks passed

mzuehlke added a commit to mzuehlke/munit-scalacheck that referenced this pull request Apr 26, 2024

update munit to 1.0.0-RC1

a0d8b65

changes because munit-diff is now an independent artifact: scalameta/munit#756 * add dependency for munit-diff * adapt imports

mzuehlke mentioned this pull request Apr 26, 2024

update munit to 1.0.0-RC1 scalameta/munit-scalacheck#12

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract diff module #756

Extract diff module #756

majk-p commented Mar 26, 2024

majk-p Mar 26, 2024

majk-p commented Mar 26, 2024

tgodzik Mar 27, 2024

majk-p Mar 28, 2024

tgodzik Mar 28, 2024

majk-p Mar 29, 2024

tgodzik Mar 29, 2024

tgodzik commented Mar 27, 2024

valencik commented Mar 29, 2024

zainab-ali Apr 2, 2024

tgodzik Apr 10, 2024

zainab-ali Apr 16, 2024

majk-p Apr 16, 2024

tgodzik Apr 16, 2024

tgodzik Apr 23, 2024

tgodzik Apr 16, 2024

tgodzik Apr 16, 2024

tgodzik Apr 23, 2024

majk-p Apr 23, 2024

tgodzik Apr 23, 2024

tgodzik commented Apr 23, 2024

majk-p commented Apr 23, 2024

tgodzik commented Apr 23, 2024

majk-p commented Apr 23, 2024

tgodzik Apr 23, 2024

majk-p Apr 23, 2024

tgodzik left a comment

zainab-ali commented Apr 24, 2024

tgodzik commented Apr 24, 2024

Extract diff module #756

Extract diff module #756

Conversation

majk-p commented Mar 26, 2024

Choose a reason for hiding this comment

majk-p commented Mar 26, 2024

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

tgodzik commented Mar 27, 2024

valencik commented Mar 29, 2024

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

tgodzik commented Apr 23, 2024

majk-p commented Apr 23, 2024

tgodzik commented Apr 23, 2024

majk-p commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

zainab-ali commented Apr 24, 2024

tgodzik commented Apr 24, 2024