feat: add framework name to the test classes BSP request #6822
Conversation
bce8a37 to
5a6ff3d
Compare
|
Main question is: am I allowed to modify already defined keys or should I create new ones and include this logic here while leaving already existing keys untouched? |
There was a problem hiding this comment.
Unfortunately we cannot change the return type of keys in the sbt 1.x series because it could break the builds and plugins using that key.
I think you can group the definedTests by framework in the BuildServerProtocol implementation directly, as suggested below.
| defined.testPerFramework.map { | ||
| case (framework, tests) => | ||
| ScalaTestClassesItem( | ||
| bspTargetIdentifier.value, | ||
| tests.map(_.name).toVector, | ||
| framework.name |
There was a problem hiding this comment.
You cannot change the type of Keys.definedTests. But here you can use loadedTestFrameworks.value to build a Map[Fingerprint, Framework] and then group each definedTests.value by framework.
There was a problem hiding this comment.
I saw your comment too late :/ I've already tweaked my initial implementation to not break compatibility by just adding other keys but I find your proposition far better, I'll change that
There was a problem hiding this comment.
The only key which s changed now is bspScalaTestClassesItem. Is changing bsp keys acceptable or just to be fully compliant it's better to create a new one?
cc: @eed3si9n
There was a problem hiding this comment.
Assuming no plugins are consuming bsp* keys, I am ok with making mima exceptions for those.
There was a problem hiding this comment.
scala-debug-adapter does not depend on bspScalaTestClassesItem and I found no consumer in https://github.com/search?p=1&q=bspScalaTestClassesItem&type=Code.
So I think we are good with this one.
There was a problem hiding this comment.
Sorry, I've never used MiMa before - if mainProj/mimaReportBinaryIssues passes should I change something with mima or is it ok?
There was a problem hiding this comment.
It is unclear to me why the mimaReportBinaryIssues passes. But since it does I think there is nothing else to do.
There was a problem hiding this comment.
Amina and I started on sbt 2.x on develop branch, so mima has been turned off. Maybe we should create 1.7.x branch off of 1.6.x?
https://github.com/sbt/sbt/blob/develop/.github/workflows/ci.yml#L102
There was a problem hiding this comment.
I just created branch 1.7.x. @kpodsiad you can repoen this PR to merge into 1.7.x.
83f37a4 to
b7b63c2
Compare
main/src/main/scala/sbt/internal/server/BuildServerProtocol.scala
Outdated
Show resolved
Hide resolved
|
There is an error in tests: but I don't know how to deal with it. |
|
You can work around this using |
b7b63c2 to
0495697
Compare
|
Superseded by #6830 |
Implementation of build-server-protocol/build-server-protocol#300
I wanted to do as few changes as possible to not break compatibility or change the codebase completely. For now, I'll create this PR as a draft to see the test results and your opinion about it.