SCL-24481: Support running separate ScalaTest tests via Bazel #709
Conversation
| psiElement match { | ||
| case leaf: LeafPsiElement if leaf.getElementType == ScalaTokenTypes.tIDENTIFIER => | ||
| leaf.getParent match { | ||
| case _: ScTypeDefinition | _: ScReferenceExpression => true |
There was a problem hiding this comment.
Wouldn't hurt adding a comment why only these 2 types are handled here
|
|
||
| import java.util | ||
|
|
||
| class BazelTestRunLineMarkerContributor extends BazelRunLineMarkerContributor { |
There was a problem hiding this comment.
(minor)
I would add a scaladoc link to org.jetbrains.plugins.scala.testingSupport.test.ui.ScalaTestRunLineMarkerProvider here as a reminder of where is the rest scala test frameworks handling is made
| false | ||
| } | ||
|
|
||
| override def getSingleTestFilter(element: PsiElement): String = |
There was a problem hiding this comment.
Hm, so the getSingleTestFilter method comes from BazelJavaRunLineMarkerContributor (and from BazelRunLineMarkerContributor) and has some default implementation.
It looks then, that it's implied that one must not implement 2 separate providers for tests and non-tests (It's hard to tell what was the original idea without the documentation).
However, currently 2 providers are implemented:
BazelScalaRunLineMarkerContributor and BazelTestRunLineMarkerContributor which means that 2 test logics will be executed, which seems "off".
cc @jastice
There was a problem hiding this comment.
You're right, I see the concern now. When I implemented this, I followed the pattern from the existing ScalaRunLineMarkerContributor and ScalaTestRunLineMarkerProvider, which are split into separate classes. I tried to keep my changes less invasive by maintaining this same structure.
However, I agree that the presence of the getSingleTestFilter method in BazelRunLineMarkerContributor does suggest that both test and non-test execution should be implemented in a single class.
Should I try to merge BazelScalaRunLineMarkerContributor and BazelTestRunLineMarkerContributor into a single contributor class?
There was a problem hiding this comment.
I asked folks from the Bazel team for their thoughts as well.
Let's wait
There was a problem hiding this comment.
Having two separate contributors will not break anything, but BazelRunLineMarkerContributor is used for both test and non-test situations. Merging these into one contributor should simplify the flow and make it easier for future maintenance
There was a problem hiding this comment.
@unkarjedy so what do you think now: should I merge contributors or keep as they are?
There was a problem hiding this comment.
Yes, please merge them given we are talking about maintaining code in Scala Plugin repository now.
We still could extract test logic calculation to separate some separate class to not to mix test and non-test logic, especially if there is a lot of code in the merged version.
The main point is to not register 2 separate providers
There was a problem hiding this comment.
@kusaeva We are currently in the end-of-the-year pre-release state with a lot of things on the table.
So please, expect some delays, we will try to handle/review it within a week.
| name.getOrElse(util.List.of[String]()) | ||
| } | ||
|
|
||
| private def escape(testName: String): util.List[String] = |
There was a problem hiding this comment.
Shouldn't the same be relevant to the regular ScalaTest runners (non-Bazel)?
Have you checked if there is any handling for it there?
There was a problem hiding this comment.
To be honest, I borrowed this part from ijwb's scala-bazel integration
I tried to look for an analog somewhere in the ScalaTestRunLineMarkerProvider, but unsuccessfully
|
Note, there are no any tests covering the functionality, which will certainly eventually lead to regressions. |
|
Meanwhile I've added support for running zio-test when the zio-intellij plugin is installed. Given that ZIO is quite popular in the Scala community, I thought this would be a useful addition. However, if you feel this is outside the scope of this project, please let me know and I'll revert the changes. |
| </extensions> | ||
| <extensions defaultExtensionNs="com.intellij"> | ||
| <runLineMarkerContributor implementationClass="org.jetbrains.plugins.scala.bazel.BazelScalaRunLineMarkerContributor" language="Scala" order="before ScalaRunLineMarkerContributor"/> | ||
| <runLineMarkerContributor implementationClass="org.jetbrains.plugins.scala.bazel.BazelTestRunLineMarkerContributor" language="Scala" order="first"/> |
There was a problem hiding this comment.
order="first" here to override zio-intellij's runLineMarkerContributor
There was a problem hiding this comment.
Again, the best would be to have it as a line-comment directly in the .xml file, not to loose this information
4 participants
Previously, support for running individual ScalaTest tests through Bazel was implemented in the Bazel plugin (ijwb). However, with the introduction of the new official Bazel plugin from JetBrains, it makes more sense to move this functionality to the Scala plugin, where test framework-specific logic naturally belongs.
With this change, Bazel users can run individual test specs using gutter icons:
Note: Running individual tests from a spec still requires some changes on the Bazel plugin side (which I hope will be accepted as well).