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

feat: add framework name to the test classes BSP request #1755

Merged
merged 3 commits into from
Apr 26, 2022

Conversation

kpodsiad
Copy link
Contributor

I've added [skip ci] because for now I only want to see what do you folks think about this PR.
This PR will stay as a draft until changes in bsp4j are merged, for now I've used locally published bsp4j version.

It is an implementation of the BSP proposal which is described here. This proposal adds an optional framework field to the returned class.

There's already implementation for bloop.

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me. Please update as appropriate and remove the draft status when you think it is mergeable. Thank you!

@kpodsiad kpodsiad force-pushed the feat/scala-test-classes branch from 5786991 to 5b3d67a Compare March 21, 2022 10:47
@kpodsiad kpodsiad changed the title feat: add framework name to the test classes BSP request [skip ci] feat: add framework name to the test classes BSP request Mar 21, 2022
build.sc Outdated Show resolved Hide resolved
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surprisingly, this is only for Scala projects and not for e.g. Java projects. Anyhow, I'm happy to merge, although this feature is probably not very relevant for Mill users, as we only support one test framework per module.

@kpodsiad
Copy link
Contributor Author

Resolution is failing because ch.epfl.scala:bsp4j:2.0.0+70-f6e47d42-SNAPSHOT is being resolved on the sonatype releases, but it's a snapshot. I'll wait until the official release is published.

@kpodsiad kpodsiad force-pushed the feat/scala-test-classes branch from af28899 to 5f6e338 Compare April 22, 2022 07:15
@kpodsiad kpodsiad marked this pull request as ready for review April 22, 2022 09:48
Copy link
Contributor Author

@kpodsiad kpodsiad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lefou I updated PR with the newly released bsp4j version.

Surprisingly, this is only for Scala projects and not for e.g. Java projects.

Do you mean that method names of method and classes contain ScalaTestClasses?

Anyhow, I'm happy to merge, although this feature is probably not very relevant for Mill users, as we only support one test framework per module.

It may be not very relevant for Mill users but this will be very handy for Metals and maybe other IDEs as well. Thanks to that Metals will be able to detect single tests for some frameworks.

Comment on lines +630 to +634
override def debugSessionStart(debugParams: DebugSessionParams)
: CompletableFuture[DebugSessionAddress] =
completable(s"debugSessionStart ${debugParams}") { state =>
throw new NotImplementedError("debugSessionStart endpoint is not implemented")
}
Copy link
Contributor Author

@kpodsiad kpodsiad Apr 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a DebugProvider capability that is not set by the Mill so it's safe to throw an error here from the BSP server perspective.

@lefou
Copy link
Member

lefou commented Apr 25, 2022

@lefou I updated PR with the newly released bsp4j version.

Surprisingly, this is only for Scala projects and not for e.g. Java projects.

Do you mean that method names of method and classes contain ScalaTestClasses?

I mean, the framework name is only relevant for Scala projects, as it is only referenced in ScalaBuildServer. So pure Java projects, for which we also sbt.testing, don't profit from this test framework selection.

@kpodsiad
Copy link
Contributor Author

@lefou It's true that it's more general than Scala and there was a small discussion about it here.

@lefou
Copy link
Member

lefou commented Apr 26, 2022

Thanks for the pointer. I think, this would be the right move, too.

@lefou lefou merged commit e183782 into com-lihaoyi:main Apr 26, 2022
@lefou lefou added this to the after 0.10.3 milestone Apr 26, 2022
kpodsiad pushed a commit to kpodsiad/metals that referenced this pull request Apr 28, 2022
… test framework name

In order to find single tests Metals needs to know test framework for give test suite. sbt/sbt#6830 which adds this feature to the sbt will be available from version 1.7.0

In mill it should be available from 0.10.4 com-lihaoyi/mill#1755
kpodsiad pushed a commit to kpodsiad/metals that referenced this pull request Apr 28, 2022
… test framework name

In order to find single tests Metals needs to know test framework for give test suite. sbt/sbt#6830 which adds this feature to the sbt will be available from version 1.7.0

In mill it should be available from 0.10.4 com-lihaoyi/mill#1755
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