-
Notifications
You must be signed in to change notification settings - Fork 21
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
--test-only #394
--test-only #394
Conversation
val testClassesData = new bsp4j.ScalaTestSuites( | ||
testClasses.filter(classes.toList.contains).map(cls => new bsp4j.ScalaTestSuiteSelection(cls, List.empty[String].asJava)).asJava, | ||
List.empty[String].asJava, | ||
List.empty[String].asJava |
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.
Consider allowing to specify specific tests.
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.
I guess it's not possible to list all test in test suites before running? I think this is less important
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.
I'm not 100% sure what bloop actually provides through bsp here. I can take a look when I have time.
Great start! I'll follow bloop development and switchover once there is a new release of mainstream 👍 |
It is now possible to specify which test suites to run using the -o or --only syntax. The tests can be specified using the class name of the test suite or by using the fully qualified name. Example: bleep test app-tests -o TestSuiteOne --only TestSuiteTwo fully qualified: bleep test app-tests -o com.foo.TestSuiteThree
@oyvindberg See the updated top comment. This works, I have not written any tests for it, but I tried it on a dummy project. The question remaining is the CLI interface. I personally don't mind the -o syntax for each suite, in practice I never use this functionality for more than one suite at a time anyway, but opinions may differ. Different syntax is not well supported by decline and requires more manual parsing on our end. |
Also if no matching test is found it runs all tests. This might not be desired and can easily be changed to no tests are run if the build has no suite matching the one passed in using --only |
Another fun and maybe useful thing we could add here is an --ignore or --not which could ignore test suites. Could be useful if you want to e.g. ignore some slow test but not move it to a separate test module |
Can we change this to what you suggested? I would be irritated if i tried to use |
fromCli.exists { fromCliCls => | ||
val fromBuildFqn = fullyQualifiedCls.split('.') | ||
val fromCliFqn = fromCliCls.split('.') | ||
fromBuildFqn.sameElements(fromCliFqn) || fromBuildFqn.last == fromCliFqn.last |
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.
I don't really like this predicate here.
to start: can we make both lists, fromBuild
and fromCli
, lowercase before we start filtering?
then let's have one rule instead of two. one of the following maybe:
- check if
fromCliFqn
is a substring offromBuildFqn
- let
fromCliFqn
be a regex and see if it matches - let
fromCliFqn
be a glob and see if it matches
or a rule like that.
then document it, at least with a description in the CLI interface
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.
I initially went with the substring approach, but after testing it some I really did not like it as I felt it became too easy for tests to clash, e.g RepositoryTest is a substring of IntegrationRepositoryTest. So my idea here was to make it as predictable as possible. It either runs whatever suites has that name e.g AppTest will run foo.AppTest and bar.AppTest and if you want to pick one you specify with fully qualified name.
I will add documentation to the CLI and bleep.build page once the predicate is decided :)
this is a great idea. maybe it could take the form of |
If no tests-suites in the project match the specified test suites provided by the user no tests will run at all. If any of the tests match, but not all the matching tests will run.
I think the --only -o syntax is good because it is close to what scala-cli, bloop and sbt uses, the --exclude -x seems very intuitive. I actually think this could be really useful for CI stuff as you could exclude tests that don't run on certain platforms and so on. |
Fixed: |
I have added support for this in the latest commit. It's a bit iffy as the -x and -o options are mutually exclusive, but this is not enforced by the CLI. |
The way this works is mutually exclusive with defining a subset of tests that only should run. It hooks into the same bsp API with the logic inverted. Instead of saying run only these test that come from the user through the cli we say run all tets besides the ones the user specified in the cli
e07f58f
to
514cece
Compare
ok, let's test this now with the current predicate. we can always iterate more later |
We only query bloop for the test suites if the -o or -x flags have been passed to the CLI
- move as much as possible to static scope (inside object) to it's testable - I struggled to follow the include/exclude logic, so I tried to make it clearer - allow combining `-o` and -`x`, latter takes precedence. - "garbage collect" `target` names (that is, project ids) so bloop gets less work to do after filtering. don't ask it to run test projects we know we're not going to run test suites in
So this is a very crude implementation that should work once scalacenter/bloop#2360 is merged and bleep uses mainline bloop.
However the cli interface is not great as it will look like bleep test project -o TestSuiteOne -o TestSuiteTwo ... and so on. I don't think it's possible to combine Option and arguments in decline? So I'm guessing here we have to use arguments and just do some parsing to make it look nice?
Update:
It is now possible to specify which test suites to run using the -o or
--only syntax.
The tests can be specified using the class name of the test suite or by
using the fully qualified name.
Example:
bleep test app-tests -o TestSuiteOne --only TestSuiteTwo
fully qualified:
bleep test app-tests -o com.foo.TestSuiteThree