Support “Pending” test case Tag #839 #840
Conversation
jnd-au
commented
Oct 3, 2024
jnd-au
commented
- Also show elapsed time for JS/Native Ignored tests (may be non-zero), to be consistent with JVM
- Also show elapsed time for JS/Native Ignored tests (may be non-zero), to be consistent with JVM
| if (annotation instanceof Tag) { | ||
| Tag tag = (Tag) annotation; | ||
| String kind = tag.getClass().getName(); | ||
| if (kind.equals("munit.Tag") && tag.value().equals("Pending")) { |
There was a problem hiding this comment.
| if (kind.equals("munit.Tag") && tag.value().equals("Pending")) { | |
| if (tag.value().equals("Pending")) { |
is this needed? We already check if it's a Tag class
There was a problem hiding this comment.
Thanks for your review! The reason for lines 143-146 was for simplicity, to avoid adding lots of Java and JS interfaces in the munit.internal.junitinterface package. However, based on your feedback, I’ll push some changes to that interface.
junit-interface/src/main/java/munit/internal/junitinterface/EventDispatcher.java
Outdated
Show resolved
Hide resolved
| @@ -1,7 +1,10 @@ | |||
| package object munit { | |||
| case class PendingComment(override val value: String) extends Tag(value) | |||
There was a problem hiding this comment.
What about having a single class instead?
| case class PendingComment(override val value: String) extends Tag(value) | |
| case class Pending(override val value: String = Pending) extends Tag(value) |
There was a problem hiding this comment.
Well, we need to differentiate the special sentinel-value flag from the arbitrary user-comment instances, so I felt that the cleanest way is with types, not string values. If we use a special string value like you suggested, then tags would be coalesced together and then we need to separate them out again, with separate implementations for Java and ScalaJS. Here’s is what the implementations might look like:
boolean isPending = false;
...
if (tag instanceof PendingTag) {
if (tag.value().equals("Pending")) {
isPending = true;
}
else {
builder.append(" ");
builder.append(tag.value());
}
}
...
if (isPending) {
builder.insert(0, " PENDING");
} val (isPending, pendingComments) = annotations.collect {
case pending: PendingTag => pending
}.partition(_.equals(munit.Pending))
isPending.distinct.map(_.value.toUpperCase) ++ pendingComments.map(_.value)This is okay, but somehow these implementations, and their equivalency, seem more unclear to me. So I didn’t push a commit for this.
| @@ -325,7 +325,7 @@ class MUnitRunner(val cls: Class[_ <: Suite], newInstance: () => Suite) | |||
| catch onError | |||
| result.map { _ => | |||
| notifier.fireTestFinished(description) | |||
| true | |||
| !test.tags(Pending) | |||
There was a problem hiding this comment.
Aren't we ignoring PendingComment here and below?
There was a problem hiding this comment.
Yes, PendingComment uses Pending (because def pending(comment: String): TestOptions = pending.tag(PendingComment(comment))), so we only need to check for Pending.
