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

--test-only #394

Merged
merged 7 commits into from
Aug 8, 2024
Merged

--test-only #394

merged 7 commits into from
Aug 8, 2024

Conversation

KristianAN
Copy link
Contributor

@KristianAN KristianAN commented Jun 28, 2024

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

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
Copy link
Contributor Author

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.

Copy link
Owner

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

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 not 100% sure what bloop actually provides through bsp here. I can take a look when I have time.

@oyvindberg
Copy link
Owner

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
@KristianAN KristianAN marked this pull request as ready for review August 5, 2024 14:47
@KristianAN
Copy link
Contributor Author

@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.

@KristianAN
Copy link
Contributor Author

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

@KristianAN KristianAN changed the title Wip: --test-only --test-only Aug 5, 2024
@KristianAN
Copy link
Contributor Author

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

@oyvindberg
Copy link
Owner

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

Can we change this to what you suggested? I would be irritated if i tried to use --only and bleep would just start to run everything without any further feedback

fromCli.exists { fromCliCls =>
val fromBuildFqn = fullyQualifiedCls.split('.')
val fromCliFqn = fromCliCls.split('.')
fromBuildFqn.sameElements(fromCliFqn) || fromBuildFqn.last == fromCliFqn.last
Copy link
Owner

@oyvindberg oyvindberg Aug 7, 2024

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 of fromBuildFqn
  • 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

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 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 :)

@oyvindberg
Copy link
Owner

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

this is a great idea. maybe it could take the form of --include / -i (instead of -o) or --exclude / -x for test suites maybe?

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.
@KristianAN
Copy link
Contributor Author

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

this is a great idea. maybe it could take the form of --include / -i (instead of -o) or --exclude / -x for test suites maybe?

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.

@KristianAN
Copy link
Contributor Author

KristianAN commented Aug 7, 2024

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

Can we change this to what you suggested? I would be irritated if i tried to use --only and bleep would just start to run everything without any further feedback

Fixed:
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.

@KristianAN
Copy link
Contributor Author

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

this is a great idea. maybe it could take the form of --include / -i (instead of -o) or --exclude / -x for test suites maybe?

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
@oyvindberg
Copy link
Owner

ok, let's test this now with the current predicate. we can always iterate more later

KristianAN and others added 2 commits August 8, 2024 22:18
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
@oyvindberg oyvindberg merged commit 55cb481 into oyvindberg:master Aug 8, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants