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

bugfix: Allow to add jvmopts and env variables when running tests #4393

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Sep 16, 2022

Previously, only the test suite name was sent via BSP. Now we also add env variables and jvmopts.

Fixes #4374

Comment on lines +653 to +655
@Nullable jvmOptions: java.util.List[String] = null,
@Nullable env: java.util.Map[String, String] = null,
@Nullable envFile: String = null,
Copy link
Member

Choose a reason for hiding this comment

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

Just a question.

Is it bloop only?
I haven't found these params in build-server-protocol

Copy link
Member

Choose a reason for hiding this comment

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

no, it's both bloop and sbt (all build servers which support debugging) - build-server-protocol/build-server-protocol#304

Copy link
Member

Choose a reason for hiding this comment

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

thx for explanation!

Copy link
Member

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

LGTM
btw, it's not really a bugfix, more like a improvement ;)

Previously, only the test suite name was sent via BSP. Now we also add env variables and jvmopts.

Fixes scalameta#4374
@tgodzik tgodzik merged commit 42c5b16 into scalameta:main Sep 19, 2022
@tgodzik tgodzik deleted the allow-env-tests branch September 19, 2022 12:37
@kpodsiad
Copy link
Member

Hmm, I wonder if this logic should also be applied to the requests from the Test Explorer.

def resolveTestSelectionParams(
request: ScalaTestSuitesDebugRequest
)(implicit ec: ExecutionContext): Future[b.DebugSessionParams] = {
val makeDebugSession = () =>
buildTargets.info(request.target) match {
case Some(buildTarget) =>
val debugSession =
if (supportsTestSelection(request.target))
new b.DebugSessionParams(
singletonList(buildTarget.getId),
DebugProvider.ScalaTestSelection,
request.requestData.toJson,
)
else
new b.DebugSessionParams(
singletonList(buildTarget.getId),
b.DebugSessionParamsDataKind.SCALA_TEST_SUITES,
request.requestData.suites.map(_.className).toJson,
)
Future.successful(debugSession)
case None =>
val error = BuildTargetNotFoundException(request.target.getUri)
reportErrors(error)
Future.failed(error)
}

@tgodzik
Copy link
Contributor Author

tgodzik commented Sep 19, 2022

Hmm, I wonder if this logic should also be applied to the requests from the Test Explorer.

def resolveTestSelectionParams(
request: ScalaTestSuitesDebugRequest
)(implicit ec: ExecutionContext): Future[b.DebugSessionParams] = {
val makeDebugSession = () =>
buildTargets.info(request.target) match {
case Some(buildTarget) =>
val debugSession =
if (supportsTestSelection(request.target))
new b.DebugSessionParams(
singletonList(buildTarget.getId),
DebugProvider.ScalaTestSelection,
request.requestData.toJson,
)
else
new b.DebugSessionParams(
singletonList(buildTarget.getId),
b.DebugSessionParamsDataKind.SCALA_TEST_SUITES,
request.requestData.suites.map(_.className).toJson,
)
Future.successful(debugSession)
case None =>
val error = BuildTargetNotFoundException(request.target.getUri)
reportErrors(error)
Future.failed(error)
}

Can you set addtional options for Test Explorer?

@kpodsiad
Copy link
Member

Can you set addtional options for Test Explorer?

There is some mechanism for configuration/options but currently vscode doesn't use it. Sounds like an improvement.

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.

Env variables not propogated to tests
3 participants