-
Notifications
You must be signed in to change notification settings - Fork 90
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
Support “Pending” test case Tag #839 #840
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't we ignoring PendingComment here and below? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, PendingComment uses Pending (because |
||
} | ||
} | ||
|
||
|
@@ -361,6 +361,8 @@ class MUnitRunner(val cls: Class[_ <: Suite], newInstance: () => Suite) | |
notifier.fireTestAssumptionFailed(new Failure(description, f)) | ||
case TestValues.Ignore => | ||
notifier.fireTestIgnored(description) | ||
case _ if test.tags(Pending) => | ||
notifier.fireTestIgnored(description) | ||
case _ => | ||
() | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,7 +1,10 @@ | ||||||
package object munit { | ||||||
case class PendingComment(override val value: String) extends Tag(value) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about having a single class instead?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
|
||||||
val Ignore = new Tag("Ignore") | ||||||
val Only = new Tag("Only") | ||||||
val Flaky = new Tag("Flaky") | ||||||
val Fail = new Tag("Fail") | ||||||
val Pending = new Tag("Pending") | ||||||
val Slow = new Tag("Slow") | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed? We already check if it's a Tag class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.