Skip to content

Commit ccabaec

Browse files
hinermhinerm
committed
Further simplify error messages
This change completely removes the individual matcher failures from the top-level error reporting, instead standardizing to just report the failed request. To avoid a total loss of information we special-case some failures (duplicate Ops). Note that this does require generalizing some of the exception tests (although this could be expanded if there was an easy way to capture debug log output in the tests). Also removes empty lines between Op descriptions (when present) and parameters.
1 parent d27f826 commit ccabaec

File tree

5 files changed

+53
-22
lines changed

5 files changed

+53
-22
lines changed

‎scijava-ops-engine/src/main/java/org/scijava/ops/engine/impl/DefaultOpEnvironment.java‎

Lines changed: 38 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -385,16 +385,47 @@ private <T> RichOp<T> findOp(final String opName, final Nil<T> specialType,
385385
return wrapOp(instance, conditions);
386386
}
387387
catch (OpMatchingException e) {
388-
// Try and suggest close alternatives to the request
388+
// Report the full exception trace for debugging purposes
389+
log.debug("Op matching failed.", e);
390+
391+
var debugText = "See debugging output for full failure report.";
392+
393+
if (e instanceof DependencyMatchingException) {
394+
// Preserve the specificity of the match but point to the original
395+
// request instead of the dependency, and note the debug logging
396+
throw new DependencyMatchingException(
397+
"Error matching dependencies for request:\n\n" + request + "\n\n" +
398+
debugText);
399+
}
400+
401+
var failedRequest = "\n\n" + request + "\n\n";
402+
403+
// The directly suppressed exceptions will be from the individual matchers
404+
// Check here for special cases of match failure
405+
for (Throwable t : e.getSuppressed()) {
406+
// Duplicate ops detected
407+
if (t.getMessage().startsWith("Multiple") && t.getMessage().contains(
408+
"ops of priority"))
409+
{
410+
throw new OpMatchingException(
411+
"Multiple ops of equal priority detected for request:" +
412+
failedRequest + "\n" + debugText);
413+
}
414+
}
415+
389416
var msg = helpVerbose(request);
417+
// If we have some alternatives we can suggest them here
390418
if (!msg.equals(OpDescriptionGenerator.NO_OP_MATCHES)) {
391-
msg = "No matching Ops were found. Perhaps you meant one of these:\n" +
392-
msg + "\n";
419+
msg = OpDescriptionGenerator.NO_OP_MATCHES + failedRequest +
420+
"Perhaps you meant one of these:\n" + msg + "\n\n";
393421
}
394-
// Report the full exception trace for debugging purposes
395-
log.debug("Op matching failed", e);
396-
// Create a new exception with suggested Op text to avoid an overwhelming
397-
// stack trace
422+
else {
423+
// Otherwise, not much to report
424+
msg += failedRequest;
425+
}
426+
427+
msg += debugText;
428+
398429
throw new OpMatchingException(msg);
399430
}
400431
}

‎scijava-ops-engine/src/main/java/org/scijava/ops/engine/util/Infos.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,11 @@ public static String describe(final OpInfo info) {
148148
// Step 2: Description (if present)
149149
if (!info.description().isEmpty()) {
150150
var desc = info.description().replaceAll("\n", "\n\t");
151-
sb.append("\n\t").append(desc).append("\n");
151+
// Each input prepends \n\t so we don't want to end with newlines.
152+
if (desc.endsWith("\n\t")) {
153+
desc = desc.substring(0, desc.lastIndexOf("\n\t"));
154+
}
155+
sb.append("\n\t").append(desc);
152156
}
153157

154158
// Step 3: Inputs

‎scijava-ops-engine/src/test/java/org/scijava/ops/engine/impl/DefaultOpDescriptionGeneratorTest.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,8 +136,7 @@ public void testDescriptionWithDescription() {
136136
String actual = ops.op("math.add").helpVerbose();
137137
String expected = "math.add:\n" +
138138
"\t- org.scijava.ops.engine.impl.DefaultOpDescriptionGeneratorTest$functionAdder\n" +
139-
"\t\t" + TEST_DESC + "\n" + "\t\n" +
140-
"\t\t> input1 : java.lang.Integer\n" +
139+
"\t\t" + TEST_DESC + "\n" + "\t\t> input1 : java.lang.Integer\n" +
141140
"\t\t> input2 : java.lang.Integer\n" + "\t\tReturns : java.lang.Integer";
142141
// Assert that helpVerbose shows the description as a part of the result.
143142
Assertions.assertEquals(expected, actual);

‎scijava-ops-engine/src/test/java/org/scijava/ops/engine/matcher/OpMatchingExceptionTest.java‎

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
import org.scijava.ops.api.OpMatchingException;
4040
import org.scijava.ops.engine.DependencyMatchingException;
4141
import org.scijava.ops.engine.AbstractTestEnvironment;
42+
import org.scijava.ops.engine.OpDescriptionGenerator;
4243
import org.scijava.ops.engine.adapt.functional.ComputersToFunctionsViaFunction;
4344
import org.scijava.ops.engine.create.CreateOpCollection;
4445
import org.scijava.ops.spi.Op;
@@ -82,11 +83,9 @@ public void duplicateErrorRegressionTest() {
8283
Assertions.fail();
8384
}
8485
catch (OpMatchingException e) {
85-
Assertions.assertTrue(e.getMessage().startsWith("No match found!"));
86-
Assertions.assertTrue(Arrays.stream(e.getSuppressed()).anyMatch(s -> s
87-
.getMessage().startsWith("Multiple 'test.duplicateOp/" +
88-
"java.util.function.Function<java.lang.Double, java.lang.Double>' " +
89-
"ops of priority 0.0:")));
86+
Assertions.assertTrue(e.getMessage().startsWith(
87+
"Multiple ops of equal priority detected for request"));
88+
Assertions.assertTrue(e.getMessage().contains("test.duplicateOp"));
9089
}
9190
}
9291

@@ -102,7 +101,8 @@ public void missingDependencyRegressionTest() {
102101
}
103102
catch (DependencyMatchingException e) {
104103
String message = e.getMessage();
105-
Assertions.assertTrue(message.contains("Name: \"test.nonexistingOp\""));
104+
Assertions.assertTrue(message.contains(
105+
"Name: \"test.missingDependencyOp\""));
106106
}
107107
}
108108

@@ -118,9 +118,7 @@ public void missingNestedDependencyRegressionTest() {
118118
}
119119
catch (DependencyMatchingException e) {
120120
String message = e.getMessage();
121-
Assertions.assertTrue(message.contains(
122-
"Name: \"test.missingDependencyOp\""));
123-
Assertions.assertTrue(message.contains("Name: \"test.nonexistingOp\""));
121+
Assertions.assertTrue(message.contains("Name: \"test.outsideOp\""));
124122
}
125123
}
126124

@@ -137,8 +135,7 @@ public void missingDependencyViaAdaptationTest() {
137135
}
138136
catch (DependencyMatchingException e) {
139137
String message = e.getMessage();
140-
Assertions.assertTrue(message.contains("Adaptor:"));
141-
Assertions.assertTrue(message.contains("Name: \"test.nonexistingOp\""));
138+
Assertions.assertTrue(message.contains("Name: \"test.adaptMissingDep\""));
142139

143140
}
144141
}

‎scijava-ops-engine/src/test/java/org/scijava/ops/engine/matcher/adapt/OpAdaptationInfoTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ public void testAdaptedDescription() {
8282
"org.scijava.ops.engine.matcher.adapt.OpAdaptationInfoTest.adaptableOp(java.lang.Double,java.lang.Double)\n\t" +
8383
"Adaptor: org.scijava.ops.engine.adapt.functional.FunctionsToComputers$Function2ToComputer2\n\t\t" +
8484
"Depends upon: org.scijava.ops.engine.copy.CopyOpCollection$copyDoubleArray\n\t" //
85-
+ TEST_DESC + "\n\n\t" //
85+
+ TEST_DESC + "\n\t" //
8686
+ "> input1 : java.lang.Double\n\t" //
8787
+ "> input2 : java.lang.Double\n\t" //
8888
+ "> output1 : @CONTAINER double[]";

0 commit comments

Comments
 (0)