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

[Proposal] Add more detailed Scala Test Classes Request #296

Closed
kpodsiad opened this issue Feb 16, 2022 · 4 comments
Closed

[Proposal] Add more detailed Scala Test Classes Request #296

kpodsiad opened this issue Feb 16, 2022 · 4 comments

Comments

@kpodsiad
Copy link
Contributor

kpodsiad commented Feb 16, 2022

Proposition

Currently, mentioned request returns

export interface ScalaTestClassesResult {
  items: ScalaTestClassesItem[];

  /** An optional id of the request that triggered this result. */
  originId?: String;
}

export interface ScalaTestClassesItem {
  /** The build target that contains the test classes. */
  target: BuildTargetIdentifier;

  /** The fully qualified names of the test classes in this target */
  classes: String[];
}

and I want to focus on ScalaTestClassesItem.classes. It consists of fully qualified names, there is no additional information about e.g. test framework to which the given test class belongs. It's fine until BSP client uses other BSP endpoints to execute test such as test request or debug request. However performing some action which requires knowledge about test framework by BSP client is very challenging - client has to somehow determine test framework of the given test class.

In order to solve that issue, aforementioned request can be slightly altered in order to contain such information:

case class ScalaTestSuitesResult(
  items: List[ScalaTestSuitesItem]
)
case class ScalaTestSuitesItem(
    target: BuildTargetIdentifier,
    suites: List[ScalaTestFrameworkSuites]
)
case class ScalaTestFrameworkSuites(
    framework: String,
    // Fully qualified names of test classes
    classes: List[String]
)

Since most (if not even all) known to me BSP servers uses sbt-test-interface under the hood it is reasonable to define value of ScalaTestFrameworkSuites.framework field to be the value of Framework.name. Then BSP Client can perform some framework-specific logic based on this field value.

Motivation

Metals and test explorer are concrete example of how knowing test framewor could be used:

  • it will allow more fine grained discovery of tests: e.g. search for @Test annotations for junit. Different strategy has to be applied for each framework.
  • it will allow Metals to implement their own TestRunner. Currently for running tests Metals are using debug request with no breakpoints. Implementing TestRunner in Metals will decrease number of moving parts and reduce overhead related with Debug Adapter Protocol stuff which will result in a better user experience.

Implementation difficulty

I've looked at bloop, sbt and mill to estimate if this endpoint can be implemented in each of them - it's doable and it seems to not be difficult. I've even implemented this endpoint for bloop already as a PoC.

@tgodzik
Copy link
Collaborator

tgodzik commented Feb 16, 2022

I think this is a great idea. I think Intellij could also benefit from that information, no? CC @jastice

@adpi2
Copy link
Member

adpi2 commented Feb 16, 2022

This proposal makes perfect sense to me.

In term of implementation I would suggest a slightly different format:

case class ScalaTestSuitesResult(
  items: List[ScalaTestSuitesItem]
)
case class ScalaTestSuitesItem(
    target: BuildTargetIdentifier,
    framework: Option[String],
    suites: List[ScalaTestFrameworkSuites]
)

First because I think it is more simple to have one layer (ScalaTestSutiesItem) rather than two (ScalaTestSuitesItem and ScalaTestFrameworkSuites).

More importantly it maintains the compatibility: Newer BSP servers can respond to older clients with this format and newer clients can read this format from older servers.
The framework field would be optional for this reason only, but it should be expected in newer implementations.

@kpodsiad
Copy link
Contributor Author

Yeah, maintaining compatibility might be easier to handle on both sides, client and server, than a brand new endpoint. Thanks @adpi2!

kpodsiad added a commit to scalacenter/bloop that referenced this issue Mar 14, 2022
…1695)

Build client can use information about the test framework of tests to provide better UX when running/debugging tests.
More information can be found at build-server-protocol/build-server-protocol#296 (comment).
lefou pushed a commit to com-lihaoyi/mill that referenced this issue Apr 26, 2022
It is an implementation of the BSP proposal which is described in build-server-protocol/build-server-protocol#296 (comment). This proposal adds an optional framework field to the returned class.

There's already implementation for bloop: scalacenter/bloop#1695

Also added `debugSessionStart` endpoint.

Pull request: #1755
@kpodsiad
Copy link
Contributor Author

closed via #300

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

No branches or pull requests

3 participants