improve exception messages used within import dialog #314

cosmicexplorer · 2017-09-08T16:00:59Z

Provide better exception messages when PantsUtil.listAllTargets() fails.
- e.g. whether the process finished with an exit code or was interrupted
Log the exit code and output file if the process ran to completion.

Currently, the function throws a PantsException on failure which describes the command line that failed. This command line already contains the output file, incidentally, but this change makes the output file explicit.

when .listAllTargets() fails: - log the command line - provide better exception messages for failure cases

cosmicexplorer · 2017-09-08T16:07:26Z

CI failed because of the md5sum, just uploaded another commit to fix that. Since .listAllTargets() is called exactly once in an integration test, I want to add some unit tests that actually ensure these exceptions are thrown.

wisechengyi

Thanks! Look mostly good with some aspects to address.

wisechengyi · 2017-09-11T20:09:06Z

common/com/twitter/intellij/pants/util/PantsUtil.java

+                        processOutput.getExitCode()) +
+          String.format("argv: '%s'\n", cmd.getCommandLineString()) +
+          String.format("stdout:\n%s\n", listOutput) +
+          String.format("warnings:\n%s\n", processOutput.getStdout()) +


if the command fails, nothing will be in listOutput.

so stdout will be processOutput.getStdout() and stderr will be processOutput.getStderr()

That makes sense -- I've made the exception message contain just stdout and stderr as you described.

wisechengyi · 2017-09-11T20:10:17Z

common/com/twitter/intellij/pants/util/PantsUtil.java

+          String.format("argv: '%s'\n", cmd.getCommandLineString()) +
+          String.format("stdout:\n%s\n", listOutput) +
+          String.format("warnings:\n%s\n", processOutput.getStdout()) +
+          String.format("stderr:\n%s\n", processOutput.getStderr());


nit: instead of \n on every line, put them in an array then join with \n

wisechengyi · 2017-09-11T20:10:51Z

common/com/twitter/intellij/pants/util/PantsUtil.java

      }
      else {
-        throw new PantsException("Failed:" + cmd.getCommandLineString());
+        final String errorDescription =
+          String.format("could not list targets: pants exited with status %d\n",


nit: capitalization Could not...... Pants ...

Fixed, also fixed in the other exception message to "Could not execute command...".

wisechengyi · 2017-09-11T20:13:11Z

common/com/twitter/intellij/pants/util/PantsUtil.java

+        String.format("could not execute command: '%s' due to error: '%s'",
+                      cmd.getCommandLineString(),
+                      e.getMessage());
+      LOG.warn(noProcessErrMsg, e);


nit:
1.noProcessErrMsg may suggests boolean, maybe otherProcessErrMsg
2. capitalization in error message

Changed to processCreationFailureMessage and fixed capitalization.

master currently fails the test suite, because jetbrains has removed the previous version of IDEA for download from their site. This pull request makes the changes listed below so master can run again. This issue was blocking #312 and #314. - bump ij version to the [most recent available release](https://www.jetbrains.com/idea/download) - update list of jars read from the scala plugin directory for the new version of ij - update checksums for IDEA and plugins - update scala sdk version to 2.11.11 - explicitly specify python and jdk version in `.travis.yml` - refactor some calls that were marked deprecated in between previous and current versions of ij

cosmicexplorer · 2017-10-03T08:31:06Z

I removed .loadTargets() and added the build file path check to PantsUtil.listAllTargets(), which allowed me to test both the PantsUtil function as well as the UI action. This and the previous commit both pass CI.

I still don't know how to make a test checking whether the project import dialog does actually get closed after ./pants list ... fails. There may be some hook that runs when the dialog is dismissed, but I'm not sure. I think that's a lot of effort to do in this pull request, but I'll try to figure out ways to do that in the future.

wisechengyi

looks good. Thanks @cosmicexplorer ! just a few points

wisechengyi · 2017-10-03T15:14:54Z

tests/com/twitter/intellij/pants/util/PantsUtilTest.java

+    File projectDir = getProjectFolder();
+    VirtualFile virtualProjectDir = LocalFileSystem.getInstance().findFileByIoFile(projectDir);
+    Optional<VirtualFile> pantsIniFileResult = PantsUtil.findPantsIniFile(Optional.ofNullable(virtualProjectDir));
+    assertTrue(pantsIniFileResult.isPresent());


Could you add appropriate error messages for all assertions?

Done, here and elsewhere.

wisechengyi · 2017-10-03T15:17:17Z

tests/com/twitter/intellij/pants/util/PantsUtilTest.java

+    File nonexistentFile = new File(nonexistentFilePath);
+    assertTrue(!nonexistentFile.exists());
+    assertTrue(!PantsUtil.isBUILDFilePath(nonexistentFilePath));
+    assertEquals(PantsUtil.listAllTargets(nonexistentFilePath),


Generally speaking, each test function should focus on one method instead of covering all the methods. Otherwise if you change the behavior of some function in the future, it requires modifying multiple test functions. E.g.

void testBuildFilePath() { assertTrue("msg", PantsUtil.isBUILDFilePath(...)) assertFalse("msg", PantsUtil.isBUILDFilePath(...)) } void testListTargets() { ... }

Let me know if you would prefer to do so with a separate patch.

That makes sense. I'll fix that in this file and in PantsProjectSettingsTest.java.

wisechengyi · 2017-10-03T15:22:36Z

tests/com/twitter/intellij/pants/util/PantsUtilTest.java

+    String pantsIniFilePath = pantsIniFileResult.get().getPath();
+    File pantsIniFile = new File(pantsIniFilePath);
+    assertTrue(pantsIniFile.exists() && !pantsIniFile.isDirectory());
+    assertTrue(!PantsUtil.isBUILDFilePath(pantsIniFilePath));


use assertFalse instead, ditto for other cases.

Done, here and elsewhere.

wisechengyi · 2017-10-03T17:49:17Z

tests/com/twitter/intellij/pants/util/PantsUtilTest.java

+
+  public void testListTargetInvalidBuildFile() {
+    String projectDir = getProjectFolder().getPath();
+    Path invalidBuildFilePath = Paths.get(


can probably use something like https://github.com/wisechengyi/intellij-pants-plugin/blob/540e81a0b001aac8bbdef23dd966804d60cc3f4f/testFramework/com/twitter/intellij/pants/testFramework/PantsTestUtils.java#L16 to get testData

Gotcha, I'll try to make this cleaner.

wisechengyi · 2017-10-03T17:52:12Z

tests/com/twitter/intellij/pants/util/PantsUtilTest.java

+    } catch (PantsException e) {
+      caught = true;
+    }
+    assertTrue("PantsException not thrown for invalid BUILD file", caught);


you can put the assertion/fail message in the try block, e.g. https://github.com/wisechengyi/intellij-pants-plugin/blob/e01fd6db7c00636108cb95f12bb8bef7fc853e59/tests/com/twitter/intellij/pants/model/PantsOptionsTest.java#L53-L61

so no need for the caught variable.

Thanks for the example, I hadn't thought of that. I'll make the change.

wisechengyi

Thanks @cosmicexplorer . just one nit

wisechengyi · 2017-10-05T19:15:48Z

tests/com/twitter/intellij/pants/util/PantsUtilTest.java

+
+    try {
+      PantsUtil.listAllTargets(invalidBuildFilePath);
+      fail(String.format("%s should have been thrown"));


is there a particular exception to be caught? what is %s for?

Whoops. I used String.format() following the example you gave in your previous comment, but forgot that means I need to check it. Just pushed a change fixing this.

# Notable Changes - Compatibility with IntelliJ 2017.3. #319 - Add .idea, .pants.d to git ignore. #307 - Improve exception messages used within import dialog. #314, #321 - Update release doc. #306 - Add a lint action to the plugin context menus and dropdown menu. #312 - Fix a bug which would occur when displaying two projects at once. #320 - Attach annotations for plugin selected JDK. #308

Daniel McClanahan added 2 commits September 8, 2017 09:00

improve error messages in .listAllTargets()

ee86ef4

when .listAllTargets() fails: - log the command line - provide better exception messages for failure cases

add new scala plugin checksum

097f842

cosmicexplorer changed the title ~~improve exception messages used within import dialog~~ improve exception messages used within import dialog [WIP] Sep 8, 2017

cancel import after failed pants list

6935c3e

wisechengyi reviewed Sep 11, 2017

View reviewed changes

cosmicexplorer mentioned this pull request Oct 2, 2017

update ij version and checksums #316

Merged

Daniel McClanahan added 8 commits October 2, 2017 12:21

Merge branch 'master' into dmcclanahan/improve-error-message-startup

3f0cdd9

clarify purpose of new test data

ec4a31f

revert unnecessary formatting change

d7ce35d

address review comments

9192636

fix compile errors

7a902ce

refactor .listAllTargets() and add tests for changes

abda473

fix relative path to invalid-build-file

ffecffe

remove unnecessary name change

c04a237

cosmicexplorer changed the title ~~improve exception messages used within import dialog [WIP]~~ improve exception messages used within import dialog Oct 3, 2017

wisechengyi reviewed Oct 3, 2017

View reviewed changes

Daniel McClanahan added 4 commits October 5, 2017 03:18

address review comments

21e450b

refactor invalid import targets (tests should FAIL!)

87c96f8

remove intentional failing assertion in setUp()

60d650a

add verification of exception message

da5f43b

wisechengyi approved these changes Oct 5, 2017

View reviewed changes

Daniel McClanahan added 2 commits October 5, 2017 12:33

fix exception message assertion and ensure exception class shown

429f25b

make assertion message more clear

2ad0787

wisechengyi merged commit 2dc6913 into pantsbuild:master Oct 5, 2017

cosmicexplorer mentioned this pull request Oct 5, 2017

add lint action #312

Merged

cosmicexplorer mentioned this pull request Dec 6, 2017

Prepare the 1.8.0 release #322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve exception messages used within import dialog #314

improve exception messages used within import dialog #314

cosmicexplorer commented Sep 8, 2017

cosmicexplorer commented Sep 8, 2017

wisechengyi left a comment

wisechengyi Sep 11, 2017

cosmicexplorer Oct 2, 2017

wisechengyi Sep 11, 2017

cosmicexplorer Oct 2, 2017

wisechengyi Sep 11, 2017

cosmicexplorer Oct 2, 2017

wisechengyi Sep 11, 2017

cosmicexplorer Oct 2, 2017

cosmicexplorer commented Oct 3, 2017

wisechengyi left a comment

wisechengyi Oct 3, 2017

cosmicexplorer Oct 4, 2017

wisechengyi Oct 3, 2017

cosmicexplorer Oct 4, 2017

wisechengyi Oct 3, 2017

cosmicexplorer Oct 4, 2017

wisechengyi Oct 3, 2017

cosmicexplorer Oct 4, 2017

wisechengyi Oct 3, 2017

cosmicexplorer Oct 4, 2017

wisechengyi left a comment

wisechengyi Oct 5, 2017

cosmicexplorer Oct 5, 2017

improve exception messages used within import dialog #314

improve exception messages used within import dialog #314

Conversation

cosmicexplorer commented Sep 8, 2017

cosmicexplorer commented Sep 8, 2017

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

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

cosmicexplorer commented Oct 3, 2017

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

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

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