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

Prettify the stdlib testing framework #542

Merged
merged 14 commits into from
Oct 1, 2024
Merged

Conversation

jiribenes
Copy link
Contributor

@jiribenes jiribenes commented Aug 16, 2024

Motivation

I'm playing around a lot with the stdlib testing framework and it would be nice for it to be ever-so-slightly prettier. :)
I was very inspired by Bun when making this.

Features

Note

This PR might still need some refactoring before it gets merged. Nitpicks are welcome.
See Caveats below.

  • string now has many more ANSI escapes for text formatting (bold, italic, etc.)
  • test now has:
    • a tiny formatting interface using which we can turn all highlighting off (useful for CI / older terminals)
    • a few combinators for printing pretty text in bold, italic, etc.
    • a formatter for time durations in ms
  • related: Add 'process' module with 'exit' function #541 enables a wrapper which makes everything much easier -- there we could make another one which does not use any formatting [= this is blocked on Add 'process' module with 'exit' function #541 which should get merged first]
  • tests for test are now in stdlib/test instead of pos (which also means they don't run on MLton, I think)

Screenshots

Before:

Screenshot 2024-08-16 at 16 28 10

After:

Screenshot 2024-08-16 at 16 28 55

Caveats

  • I don't quite like the ad-hoc formatting library: where could we move this?
    • Also, it is very ad-hoc: RESET deletes all formatting, which means that something like red("Hello " ++ bold("world") ++ "!") will actually result in <RED>Hello <BOLD>world<RESET>!<RESET>, so the exclamation mark is surprisingly no longer red.
  • Can/should we move the Duration-related things into bench? EDIT: I've moved most of them except for diff
  • Are we okay with and instead of + and -? Can we trust old terminals/CI with these?
  • Should we handle Formatter straight-away or just expose it to the user and let them handle it?
  • The timing needs to be opt-in only (= not in the CI version, sadly, since we're comparing text!) EDIT: Solved.

TODO

Comment on lines 248 to 279
val CSI = "\u001b["

def escape(s: String) = CSI ++ s ++ "m"

val BLACK = escape("30")
val RED = escape("31")
val GREEN = escape("32")
val YELLOW = escape("33")
val BLUE = escape("34")
val MAGENTA = escape("35")
val CYAN = escape("36")
val WHITE = escape("37")

val BG_BLACK = escape("40")
val BG_RED = escape("41")
val BG_GREEN = escape("42")
val BG_YELLOW = escape("43")
val BG_BLUE = escape("44")
val BG_MAGENTA = escape("45")
val BG_CYAN = escape("46")
val BG_WHITE = escape("47")

val RESET = escape("0")

val BOLD = escape("1")
val FAINT = escape("2")
val ITALIC = escape("3")
val UNDERLINE = escape("4")
val BLINK = escape("5")
val REVERSE = escape("7")
val CROSSOUT = escape("9")
val OVERLINE = escape("53")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On one hand, I'd like to leave this split up, on the other, I'm a little worried that this doesn't get inlined.
The LLVM backend cannot even compile simple top-level constants (see #496), much less this :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe currently this will not be constant-folded. I think we should support top-level constants in LLVM for this to work; WDYT @phischu?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with inlining the calls to escape and ++ here manually, but I'd still like #496. Otherwise this code is still unusable in LLVM.

@jiribenes

This comment was marked as resolved.

@jiribenes

This comment was marked as resolved.

@jiribenes
Copy link
Contributor Author

Rebased on master after #541 got merged.

Comment on lines +185 to 193
/// Recommended for standalone test files ran by CI.
///
/// Exits after running all tests:
/// - if all tests succeed, exits the program with success (exit code 0)
/// - otherwise exits the program with failure (exit code 1)
def mainSuite(name: String) { body: => Unit / { Test, Formatted } }: Unit = {
val result = suite(name, true) { body }
val exitCode = if (result) 0 else 1
exit(exitCode)
}
Copy link
Contributor Author

@jiribenes jiribenes Aug 30, 2024

Choose a reason for hiding this comment

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

Unfortunately, this cool exiting runner is pretty useless if one of your files has multiple test suites (for example here is an example "in the wild").
In general, having multiple test suites is this testing framework's weak spot.

We could mandate suites and use them just for compartmentalisation/grouping, making a new runTests handler, but what if I really just want to write three tests and be done with it all?

@jiribenes
Copy link
Contributor Author

jiribenes commented Aug 30, 2024

Not sure if we want something like the ability to set tests as ignored/skipped when developing; it feels a little bit out of scope of this PR.
Nevertheless if we ever do want to support skips, here's a rough patch:

Patch for setting a test as skipped
diff --git i/libraries/common/test.effekt w/libraries/common/test.effekt
index c30f305c..28644bea 100644
--- i/libraries/common/test.effekt
+++ w/libraries/common/test.effekt
@@ -94,6 +94,7 @@ def assertEqual[A](obtained: A, expected: A) { equals: (A, A) => Bool } { show:
 interface Test {
   def success(name: String, duration: Int): Unit
   def failure(name: String, msg: String, duration: Int): Unit
+  def skipped(name: String): Unit
 }
 
 /// Runs the `body` as a test under the given `name`
@@ -114,6 +115,10 @@ def test(name: String) { body: => Unit / Assertion } = {
   }
 }
 
+/// Does *not* run the `body` as a test under the given `name`: 
+/// Useful for ignoring a test.
+def skip(name: String) { body: => Unit / Assertion } = do skipped(name)
+
 /// Run a test suite with a given `name`.
 /// - If `printTimes` is `true` (or missing), prints out time in milliseconds.
 /// - Formats automatically using ANSI escapes.
@@ -164,14 +169,26 @@ def suite(name: String, printTimes: Bool) { body: => Unit / { Test, Formatted }
         println("  " ++ msg.red)
         resume(())
       }
+
+      // 2c) Handle a skipped test
+      def skipped(name) = {
+        skipped = skipped + 1
+        println(("»" ++ " " ++ name).dim)
+        // no need to resume
+      }
     }
   }
 
+  val total = passed + failed + skipped
+
   // 3) Format the test results
   println("")
   println(" " ++ (passed.show ++ " pass").dimWhenZeroElse(passed) { green })
+  if (skipped > 0) { 
+    println(" " ++ (skipped.show ++ " skip").dim)
+  }
   println(" " ++ (failed.show ++ " fail").dimWhenZeroElse(failed) { red })
-  println(" " ++ (passed + failed).show ++ " tests total" ++ totalDuration.ms)
+  println(" " ++ total.show ++ " tests total" ++ totalDuration.ms)
 
   // 4) Return true if all tests succeeded, otherwise false
   return failed == 0

I find this interesting, since ideally, we would have some basic CLI args support for mainSuite, allowing it to run only certain tests (this way, we could have simple IDE support even to click & run all / click & run a single test).

@jiribenes jiribenes marked this pull request as ready for review August 30, 2024 18:55
@b-studios
Copy link
Collaborator

b-studios commented Sep 10, 2024

This is so nice, can't we just merge it?

Ah, I remember... Toplevel definitions.

ping @phischu

Comment on lines +48 to +50
// missing support for multibyte character escape in a string
examplesDir / "stdlib" / "test",

Copy link
Contributor Author

@jiribenes jiribenes Sep 24, 2024

Choose a reason for hiding this comment

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

This would need #544, but we can fix that later, I'm fine with MLton support being rather second-class. :)

Comment on lines 5 to 27
interface Formatted {
def supportsEscape(escape: String): Bool
}

namespace Formatted {
/// Run given block of code, allowing all formatting
def formatting[R] { prog : => R / Formatted }: R =
try { prog() } with Formatted {
def supportsEscape(escape: String) = resume(true)
}

/// Run given block of code, ignoring all formatting
def noFormatting[R] { prog : => R / Formatted }: R =
try { prog() } with Formatted {
def supportsEscape(escape: String) = resume(false)
}

def tryEmit(escape: String): String / Formatted =
if (do supportsEscape(escape)) escape else ""

def colored(text: String, colorEscape: String): String / Formatted =
tryEmit(colorEscape) ++ text ++ tryEmit(ANSI::RESET)
}
Copy link
Contributor Author

@jiribenes jiribenes Sep 24, 2024

Choose a reason for hiding this comment

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

I don't know whether this should stay here or not: should it go into a different module?
(also see the code below)

@jiribenes

This comment was marked as resolved.

@jiribenes
Copy link
Contributor Author

jiribenes commented Sep 25, 2024

Rebased on master after the newly merged #606.

@jiribenes jiribenes requested a review from b-studios October 1, 2024 13:16
Copy link
Collaborator

@b-studios b-studios left a comment

Choose a reason for hiding this comment

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

:shipit:

@jiribenes jiribenes merged commit eb0ceee into master Oct 1, 2024
2 checks passed
@jiribenes jiribenes deleted the feature/pretty-tests branch October 1, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants