-
-
Notifications
You must be signed in to change notification settings - Fork 287
Put -s in front of other parameters for the ScalaTest runner. #1412
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
Put -s in front of other parameters for the ScalaTest runner. #1412
Conversation
ScalaTest cares about the ordering between -s (class selection) and -t (test name selection). "-s -t" will select the test in the specified class with the specified name. "-t -s" will select all tests in the specified class, and all tests with the specified name. Putting the -s we derive from --test_filter at the front of the argument list rather than at the back is more likely to do what people want when they specify both --test_filter and other selector flags. It is still possible to specify -s last by passing the arguments as --test_arg=-t --test_arg=testName --test_arg=-s --test_arg=className and leaving --test_filter out.
af8b08d to
9961e03
Compare
|
I think I was originally responsible for this (or reviewing it) and I frankly didn't understand this. So, I don't think the prior behavior was intentional. Put another way, I'm in support of merging. |
|
Looks good; could we get a test for this? i.e. an |
liucijus
left a comment
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, @srdo-humio. Please add a test to prevent future regressions.
|
👍 Added tests. |
test/scala_test_testfilter/BUILD
Outdated
| scala_test( | ||
| name = "tests", | ||
| srcs = ["A.scala", "B.scala"], | ||
| tags = ["manual"] |
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.
Please move this target under test_expect_failure/ instead, where it's expected to be not passing
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.
Done. Should the shell script remain where it is, or where do I put that?
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.
Yes, leave it there, thanks!
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.
Oh, I think this shell script does not get executed - you need to include it into test_rules_scala.sh
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.
Should be done now.
|
Thanks @srdo-humio for contribution! |
ScalaTest cares about the ordering between -s (class selection) and
-t (test name selection). "-s -t" will select the test in the specified class
with the specified name. "-t -s" will select all tests in the specified
class, and all tests with the specified name.
Putting the -s we derive from --test_filter at the front of the argument
list rather than at the back is more likely to do what people want when
they specify both --test_filter and other selector flags.
It is still possible to specify -s last by passing the arguments as
--test_arg=-t --test_arg=testName --test_arg=-s --test_arg=className
and leaving --test_filter out.
fixes #1411