Include `jvm.test.junit.options` in IntelliJ test config #439

tpasternak · 2019-11-22T14:52:20Z

closes #435

wisechengyi

Thanks, Tomasz!

wisechengyi · 2019-11-22T18:21:22Z

src/com/twitter/intellij/pants/execution/PantsJUnitTestRunConfigurationProducer.java

@@ -104,6 +105,11 @@ else if (testPackage != null) {
    }

    taskSettings.setScriptParameters(StringUtil.join(scriptParameters, " "));
+    Optional<String[]> vmOptions =
+      PantsOptions.getPantsOptions(module.getProject()).flatMap(options -> options.getList("jvm.test.junit.options"));
+


Would be good to extract the string to a constant next to https://github.com/pantsbuild/intellij-pants-plugin/blob/master/common/com/twitter/intellij/pants/util/PantsConstants.java#L40?

wisechengyi · 2019-11-22T18:21:39Z

common/com/twitter/intellij/pants/model/PantsOptions.java

@@ -43,6 +44,12 @@ public boolean has(String optionName) {
    return Optional.ofNullable(options.get(optionName));
  }

+  public Optional<String[]> getList(String optionName) {


Can you kindly add some unit tests for this function?

wisechengyi

Oh sorry. The pants runner does not need this change actually because Pants is already picking up those options via the ini files.

This is should be used for IntelliJ junit/scalatest and such, which is happens at

intellij-pants-plugin/src/com/twitter/intellij/pants/execution/PantsMakeBeforeRun.java

Lines 126 to 128 in 97a31e5

    
           BeforeRunTask pantsMakeTask = new ExternalSystemBeforeRunTask(ID, PantsConstants.SYSTEM_ID); 
        
           pantsMakeTask.setEnabled(true); 
        
           runManagerImpl.setBeforeRunTasks(runConfiguration, Collections.singletonList(pantsMakeTask), false);

Basically for any config that we have to replace Build with PantsCompile.

Edit: url changed.

wisechengyi · 2019-11-25T17:32:40Z

The VM options should appear in intellij's runners (e.g. junit/scala), but should not appear in Pants' runner. Please let me know if it's still confusing.

…

On Mon, Nov 25, 2019 at 2:14 AM Tomasz Pasternak ***@***.***> wrote: Oh, I didn't notice your last. Comment. I thought you want to override it in IntelliJ runner :). If not, then it's OK, this PR shouldn't be merged. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#439?email_source=notifications&email_token=AAERTBT3LPK5TMCFEGHE5MLQVOQQRA5CNFSM4JQR2GI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFB3M4Q#issuecomment-558085746>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAERTBX5MZQKZYSGMLHXXPTQVOQQRANCNFSM4JQR2GIQ> .

tpasternak · 2019-11-25T20:27:22Z

Yes, it's clear, I just noticed your note right after I submitted the commits. I have some problems w with classpath now, because the Pants plugin needs access to JUnit plugin's classes. Hopefully will be done tomorrow.

tpasternak · 2019-11-26T14:16:32Z

Ok, now the VM params are applied to JUnit confiuration

wisechengyi

Thanks, Tomasz! Looks mostly good with some minor points.

wisechengyi · 2019-11-26T22:03:42Z

src/com/twitter/intellij/pants/execution/PantsMakeBeforeRun.java

+
+      if(runConfiguration instanceof JUnitConfiguration) {
+        Optional<String[]> os = PantsOptions.getPantsOptions(project)
+          .flatMap(options -> options.getList(PantsConstants.PANTS_OPTION_TEST_JUNIT_OPTIONS));
+        String optionsString = String.join(" ", os.orElse(new String[]{}));
+        ((JUnitConfiguration)runConfiguration).setVMParameters(optionsString);
+      }
+


This logic is needed for if (PantsUtil.isScalaRelatedTestRunConfiguration(runConfiguration)) as well. So possibly we can extract this to a helper function.

wisechengyi · 2019-11-26T22:03:57Z

tests/com/twitter/intellij/pants/execution/OSSPantsJvmRunConfigurationIntegrationTest.java

+
+

wisechengyi · 2019-11-26T22:04:29Z

tests/com/twitter/intellij/pants/execution/OSSPantsJvmRunConfigurationIntegrationTest.java

+  private ConfigurationFromContext getPantsJunitConfigurationFromContext(ConfigurationContext context) {
+    return getConfigurationFromContext(context, PantsJUnitTestRunConfigurationProducer.class);
+  }


no longer used?

Both getJunitConfigurationFromContext and getPantsJunitConfigurationFromContext are used. I Introduced getJunitConfigurationFromContext in order to test new behavior.

wisechengyi · 2019-11-26T22:07:45Z

src/com/twitter/intellij/pants/execution/PantsMakeBeforeRun.java

+        Optional<String[]> os = PantsOptions.getPantsOptions(project)
+          .flatMap(options -> options.getList(PantsConstants.PANTS_OPTION_TEST_JUNIT_OPTIONS));


This introduces a minor lag in the UI if PantsOptions wasn't cached. We could either the following options:

Show the UI Getting Pants options ...

Preemptively populate pants options up on project start.

Thanks, I haven't try it on big projects, so I didn't observe this issue. I chose option one. If the caching mechanism is already implemented, then let's let it do its job.

Looks great. Thanks!

wisechengyi · 2019-11-26T22:09:15Z

resources/META-INF/plugin.xml

@@ -20,6 +20,7 @@
  <!--Add gradle as a dep because of Pants runner requires it.-->
  <depends>com.intellij.gradle</depends>

+  <depends optional="true" config-file="pants-junit.xml">JUnit</depends>


This should be required. so optional=false or not specifying it at all.

I chose conditional dependency, because otherwise it was failing when you were using 2019.2 for development.

Now it's unconditional.

Thanks! The plugin upgrade is normally backwards incompatible, so not-optional should be fine.

tpasternak · 2019-11-27T19:27:15Z

I broke the tests by accident while I was refactoring the code. Now it should be fine

wisechengyi · 2019-11-27T19:35:52Z

That means tests are working as intended :)

tpasternak · 2019-11-27T22:01:07Z

Now it's only "Too many projects leaked:" test that fails

wisechengyi · 2019-11-28T08:32:47Z

Thanks! This looks good. Unfortunately I found a blocking issue that internally we use certain security manager settings in jvm.test.junit.options, which intellij doesn't know about and will throw an error, so we can't land this just yet.

I will give it a bit more thought how we can proceed there.

Dependency to JUnit in plugin.xml was required, because we now edit JUnitConfiguration class in PantsMakeBeforeRun, so we need this class in the PantsPlugin classpath. closes #435

It was broken while refactoring

tpasternak · 2020-07-24T09:08:28Z

@wisechengyi can we close it?

olafurpg · 2020-08-12T14:32:01Z

ping @wisechengyi

wisechengyi reviewed Nov 22, 2019

View reviewed changes

wisechengyi requested changes Nov 22, 2019

View reviewed changes

wisechengyi reviewed Nov 26, 2019

View reviewed changes

Tomasz Pasternak added 9 commits January 31, 2020 16:06

Support for lists in pants.ini

9a7298f

Moved literal "jvm.test.junit.options" to PantsConstants

06c682e

Include VM options in IntelliJ JUnit run configuration

e148189

Dependency to JUnit in plugin.xml was required, because we now edit JUnitConfiguration class in PantsMakeBeforeRun, so we need this class in the PantsPlugin classpath. closes #435

Remove whitespace

40e9eab

Add VM options to scala run configs

07215d3

Unconditional dependency to JUnit plugin

97ed5f6

Refactoring: extract setVMParameters method

80b611e

Progress indicator when retrieving Pants options

1a01617

Fix joining of VM options

6132c08

It was broken while refactoring

wisechengyi added the blocked label Feb 13, 2020

wisechengyi closed this Aug 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include `jvm.test.junit.options` in IntelliJ test config #439

Include `jvm.test.junit.options` in IntelliJ test config #439

tpasternak commented Nov 22, 2019

wisechengyi left a comment

wisechengyi Nov 22, 2019

wisechengyi Nov 22, 2019

wisechengyi left a comment •

edited

Loading

wisechengyi commented Nov 25, 2019 via email

tpasternak commented Nov 25, 2019

tpasternak commented Nov 26, 2019

wisechengyi left a comment •

edited

Loading

wisechengyi Nov 26, 2019

tpasternak Nov 27, 2019

wisechengyi Nov 26, 2019

tpasternak Nov 27, 2019

wisechengyi Nov 26, 2019

tpasternak Nov 27, 2019

wisechengyi Nov 26, 2019

tpasternak Nov 27, 2019 •

edited

Loading

wisechengyi Nov 27, 2019

wisechengyi Nov 26, 2019

tpasternak Nov 27, 2019

wisechengyi Nov 27, 2019

tpasternak commented Nov 27, 2019 •

edited

Loading

wisechengyi commented Nov 27, 2019

tpasternak commented Nov 27, 2019

wisechengyi commented Nov 28, 2019

tpasternak commented Jul 24, 2020

olafurpg commented Aug 12, 2020

	BeforeRunTask pantsMakeTask = new ExternalSystemBeforeRunTask(ID, PantsConstants.SYSTEM_ID);
	pantsMakeTask.setEnabled(true);
	runManagerImpl.setBeforeRunTasks(runConfiguration, Collections.singletonList(pantsMakeTask), false);

		Optional<String[]> os = PantsOptions.getPantsOptions(project)
		.flatMap(options -> options.getList(PantsConstants.PANTS_OPTION_TEST_JUNIT_OPTIONS));

Include jvm.test.junit.options in IntelliJ test config #439

Include jvm.test.junit.options in IntelliJ test config #439

Conversation

tpasternak commented Nov 22, 2019

wisechengyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wisechengyi left a comment • edited Loading

Choose a reason for hiding this comment

wisechengyi commented Nov 25, 2019 via email

tpasternak commented Nov 25, 2019

tpasternak commented Nov 26, 2019

wisechengyi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpasternak Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpasternak commented Nov 27, 2019 • edited Loading

wisechengyi commented Nov 27, 2019

tpasternak commented Nov 27, 2019

wisechengyi commented Nov 28, 2019

tpasternak commented Jul 24, 2020

olafurpg commented Aug 12, 2020

Include `jvm.test.junit.options` in IntelliJ test config #439

Include `jvm.test.junit.options` in IntelliJ test config #439

wisechengyi left a comment •

edited

Loading

wisechengyi left a comment •

edited

Loading

tpasternak Nov 27, 2019 •

edited

Loading

tpasternak commented Nov 27, 2019 •

edited

Loading