Fix + improve some tests regarding Ajc output (usage messages etc.)

After Ajc usage text output is filtered into its own category
IMessage.USAGE now - see commit @31b2d60b - some tests in module
'org.aspectj.ajdt.core' were failing. I fixed and also improved them a
bit.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>

5 files changed, 110 insertions, 91 deletions

diff --git a/org.aspectj.ajdt.core/src/main/java/org/aspectj/tools/ajc/Main.java b/org.aspectj.ajdt.core/src/main/java/org/aspectj/tools/ajc/Main.java
index 81ec01a42..1f29d5835 100644
--- a/org.aspectj.ajdt.core/src/main/java/org/aspectj/tools/ajc/Main.java
+++ b/org.aspectj.ajdt.core/src/main/java/org/aspectj/tools/ajc/Main.java
@@ -95,9 +95,10 @@ public class Main {
 	 * @param errors the List sink, if any, for String error messages
 	 * @param warnings the List sink, if any, for String warning messages
 	 * @param infos the List sink, if any, for String info messages
+	 * @param usages the List sink, if any, for String usage messages
 	 * @return number of messages reported with level ERROR or above
 	 */
-	public static int bareMain(String[] args, boolean useSystemExit, List fails, List errors, List warnings, List infos) {
+	public static int bareMain(String[] args, boolean useSystemExit, List fails, List errors, List warnings, List infos, List usages) {
 		Main main = new Main();
 		MessageHandler holder = new MessageHandler();
 		main.setHolder(holder);
@@ -108,6 +109,7 @@ public class Main {
 			readMessages(holder, IMessage.ERROR, false, errors);
 			readMessages(holder, IMessage.WARNING, false, warnings);
 			readMessages(holder, IMessage.INFO, false, infos);
+			readMessages(holder, IMessage.USAGE, false, usages);
 		}
 		return holder.numMessages(IMessage.ERROR, true);
 	}
@@ -295,9 +297,8 @@ public class Main {
 		final String customMessageHolder = parmInArgs(MESSAGE_HOLDER_OPTION, args);
 		if (customMessageHolder != null) {
 			try {
-				holder = (IMessageHolder) Class.forName(customMessageHolder).newInstance();
+				holder = (IMessageHolder) Class.forName(customMessageHolder).getDeclaredConstructor().newInstance();
 			} catch (Exception ex) {
-				holder = ourHandler;
 				throw new AbortException("Failed to create custom message holder of class '" + customMessageHolder + "' : " + ex);
 			}
 		}
diff --git a/org.aspectj.ajdt.core/src/test/java/org/aspectj/ajdt/ajc/AjdtCommandTestCase.java b/org.aspectj.ajdt.core/src/test/java/org/aspectj/ajdt/ajc/AjdtCommandTestCase.java
index 95bd5c4c6..952e707b8 100644
--- a/org.aspectj.ajdt.core/src/test/java/org/aspectj/ajdt/ajc/AjdtCommandTestCase.java
+++ b/org.aspectj.ajdt.core/src/test/java/org/aspectj/ajdt/ajc/AjdtCommandTestCase.java
@@ -12,7 +12,6 @@
 
 package org.aspectj.ajdt.ajc;
 
-//import org.aspectj.ajdt.internal.core.builder.AjBuildConfig;
 import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.IOException;
@@ -25,7 +24,6 @@ import java.util.ListIterator;
 import junit.framework.TestCase;
 
 import org.aspectj.ajdt.StreamPrintWriter;
-import org.aspectj.bridge.AbortException;
 import org.aspectj.bridge.CountingMessageHandler;
 import org.aspectj.bridge.IMessage;
 import org.aspectj.bridge.IMessageHolder;
@@ -136,65 +134,57 @@ public class AjdtCommandTestCase extends TestCase {
 		assertTrue("specified a file", outputWriter.getContents().contains("incremental mode only handles source files using -sourceroots"));
 	}
 
-	public void testBadOptionAndUsagePrinting() throws InvalidInputException {
-		try {
+	public void testBadOptionAndUsagePrinting() {
 			AjdtCommand.genBuildConfig(new String[] { "-mubleBadOption" }, counter);
-		} catch (AbortException ae) {
-		}
-		// usage printed by caller to genBuildConfig now...
-		assertTrue(outputWriter.getContents() + " contains? " + "Usage",
-				outputWriter.getContents().contains("-mubleBadOption"));
-
+		String outputText = outputWriter.getContents();
+		assertTrue(
+			"bad option error not found in compiler output",
+			outputText.contains("error") && outputText.contains("argument") && outputText.contains("-mubleBadOption")
+		);
+		assertFalse(
+			"usage text found in compiler output unexpectedly",
+			outputWriter.getContents().contains("AspectJ-specific options:")
+		);
 	}
 
 	public void testHelpUsagePrinting() {
-		String[] args = new String[] { "-help" };
-
-		PrintStream saveOut = System.out;
-		ByteArrayOutputStream byteArrayOut = new ByteArrayOutputStream();
-		PrintStream newOut = new PrintStream(byteArrayOut);
-		System.setOut(newOut);
-
-		try {
-			try {
-
-				AjdtCommand.genBuildConfig(args, counter);
-			} catch (AbortException ae) {
-			}
-		} finally {
-			System.setOut(saveOut);
-		}
-
-		String text = byteArrayOut.toString();
-		assertTrue(text + " contains? " + "Usage", text.contains("Usage"));
+		AjdtCommand.genBuildConfig(new String[] { "-help" }, counter);
+		String outputText = outputWriter.getContents();
+		assertTrue(
+			"usage text not found in compiler output",
+			outputText.contains("AspectJ-specific options:")
+		);
 	}
 
-	public void q() throws InvalidInputException {
-		String[] args = new String[] { "-version" };
+	public void testXHelpUsagePrinting() {
+		AjdtCommand.genBuildConfig(new String[] { "-X" }, counter);
+		String outputText = outputWriter.getContents();
+		assertTrue(
+			"usage text not found in compiler output",
+			outputText.contains("AspectJ-specific non-standard options:")
+		);
+	}
 
+	public void testVersionPrinting() {
+		// Version string is not identified as any kind of special message like usage, error, warning. Therefore, it is
+		// printed to stdOut directly in order not to be filtered out. This is why we have to check System.out directly.
+		// Attention, this is not thread-safe when running tests in parallel and another thread prints at the same time!
 		PrintStream saveOut = System.out;
-		PrintStream saveErr = System.err;
 		ByteArrayOutputStream byteArrayOut = new ByteArrayOutputStream();
-		ByteArrayOutputStream byteArrayErr = new ByteArrayOutputStream();
 		PrintStream newOut = new PrintStream(byteArrayOut);
-		PrintStream newErr = new PrintStream(byteArrayErr);
-		System.setOut(newOut);
-		System.setErr(newErr);
 
 		try {
-			try {
-
-				AjdtCommand.genBuildConfig(args, counter);
-			} catch (AbortException ae) {
+		System.setOut(newOut);
+			AjdtCommand.genBuildConfig(new String[] { "-version" }, counter);
 			}
-		} finally {
+		finally {
 			System.setOut(saveOut);
-			System.setErr(saveErr);
 		}
-
-		String text = byteArrayOut.toString();
-		// String text2 = byteArrayErr.toString();
-		assertTrue("version output does not include 'AspectJ Compiler', output was:\n'" + text + "'", text.contains("AspectJ Compiler"));
+		String outputText = byteArrayOut.toString();
+		assertTrue(
+			"AspectJ version string not found in compiler output",
+			outputText.contains("AspectJ Compiler")
+		);
 	}
 
 	public void testNonExistingLstFile() {
diff --git a/org.aspectj.ajdt.core/src/test/java/org/aspectj/ajdt/internal/core/builder/OutjarTest.java b/org.aspectj.ajdt.core/src/test/java/org/aspectj/ajdt/internal/core/builder/OutjarTest.java
index 32e8fd329..8094bfc53 100644
--- a/org.aspectj.ajdt.core/src/test/java/org/aspectj/ajdt/internal/core/builder/OutjarTest.java
+++ b/org.aspectj.ajdt.core/src/test/java/org/aspectj/ajdt/internal/core/builder/OutjarTest.java
@@ -51,7 +51,7 @@ public class OutjarTest extends AjcTestCase {
 		String[] args = new String[] {"-aspectpath", aspectjarName, "-injars", injarName, "-outjar", injarName};
 		Message error = new Message(WeaverMessages.format(WeaverMessages.OUTJAR_IN_INPUT_PATH));
 		Message fail = new Message("Usage:");
-		MessageSpec spec = new MessageSpec(null,null,newMessageList(error),newMessageList(fail),null);
+		MessageSpec spec = new MessageSpec(null,null,newMessageList(error),newMessageList(fail),null, null);
 		CompilationResult result = ajc(baseDir,args);
 //		System.out.println(result);
 		assertMessages(result,spec);
@@ -72,7 +72,7 @@ public class OutjarTest extends AjcTestCase {
 		String[] args = new String[] {"-aspectpath", aspectjarName, "-inpath", injarName, "-outjar", injarName};
 		Message error = new Message(WeaverMessages.format(WeaverMessages.OUTJAR_IN_INPUT_PATH));
 		Message fail = new Message("Usage:");
-		MessageSpec spec = new MessageSpec(null,null,newMessageList(error),newMessageList(fail),null);
+		MessageSpec spec = new MessageSpec(null,null,newMessageList(error),newMessageList(fail),null, null);
 		CompilationResult result = ajc(baseDir,args);
 //		System.out.println(result);
 		assertMessages(result,spec);
@@ -93,7 +93,7 @@ public class OutjarTest extends AjcTestCase {
 		String[] args = new String[] {"-aspectpath", aspectjarName, "-inpath", injarName, "-outjar", aspectjarName};
 		Message error = new Message(WeaverMessages.format(WeaverMessages.OUTJAR_IN_INPUT_PATH));
 		Message fail = new Message("Usage:");
-		MessageSpec spec = new MessageSpec(null,null,newMessageList(error),newMessageList(fail),null);
+		MessageSpec spec = new MessageSpec(null,null,newMessageList(error),newMessageList(fail),null, null);
 		CompilationResult result = ajc(baseDir,args);
 //		System.out.println(result);
 		assertMessages(result,spec);
diff --git a/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/AjcTestCase.java b/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/AjcTestCase.java
index a1cccba72..a8c3f5a44 100644
--- a/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/AjcTestCase.java
+++ b/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/AjcTestCase.java
@@ -297,8 +297,10 @@ public abstract class AjcTestCase extends TestCase {
 		/**
 		 * Convenience constant that matches a CompilationResult with any number of information messages, but no others.
 		 */
-		public static final MessageSpec EMPTY_MESSAGE_SET = new MessageSpec(null, Collections.EMPTY_LIST, Collections.EMPTY_LIST,
-				Collections.EMPTY_LIST, Collections.EMPTY_LIST);
+		public static final MessageSpec EMPTY_MESSAGE_SET = new MessageSpec(
+			null, Collections.EMPTY_LIST, Collections.EMPTY_LIST, Collections.EMPTY_LIST,
+			Collections.EMPTY_LIST, Collections.EMPTY_LIST
+		);
 
 		boolean ignoreInfos = true;
 		public List<AjcTestCase.Message> fails;
@@ -306,6 +308,7 @@ public abstract class AjcTestCase extends TestCase {
 		public List<AjcTestCase.Message> warnings;
 		public List<AjcTestCase.Message> errors;
 		public List<AjcTestCase.Message> weaves;
+		public List<AjcTestCase.Message> usages;
 
 		/**
 		 * Set to true to enable or disable comparison of information messages.
@@ -330,8 +333,14 @@ public abstract class AjcTestCase extends TestCase {
 		 * @param errors The set of error messages to test for - can pass null to indicate empty set.
 		 * @param fails The set of fail or abort messages to test for - can pass null to indicate empty set.
 		 */
-		public MessageSpec(List<AjcTestCase.Message> infos, List<AjcTestCase.Message> warnings,
-				List<AjcTestCase.Message> errors, List<AjcTestCase.Message> fails, List<AjcTestCase.Message> weaves) {
+		public MessageSpec(
+			List<AjcTestCase.Message> infos,
+			List<AjcTestCase.Message> warnings,
+			List<AjcTestCase.Message> errors,
+			List<AjcTestCase.Message> fails,
+			List<AjcTestCase.Message> weaves,
+			List<AjcTestCase.Message> usages
+		) {
 			if (infos != null) {
 				this.infos = infos;
 				ignoreInfos = false;
@@ -342,6 +351,7 @@ public abstract class AjcTestCase extends TestCase {
 			this.errors = ((errors == null) ? Collections.<AjcTestCase.Message>emptyList() : errors);
 			this.fails = ((fails == null) ? Collections.<AjcTestCase.Message>emptyList() : fails);
 			this.weaves = ((weaves == null) ? Collections.<AjcTestCase.Message>emptyList() : weaves);
+			this.usages = ((weaves == null) ? Collections.<AjcTestCase.Message>emptyList() : usages);
 		}
 
 		/**
@@ -349,7 +359,7 @@ public abstract class AjcTestCase extends TestCase {
 		 * presence of any fail or abort messages in a CompilationResult will be a test failure.
 		 */
 		public MessageSpec(List<AjcTestCase.Message> infos, List<AjcTestCase.Message> warnings, List<AjcTestCase.Message> errors) {
-			this(infos, warnings, errors, null, null);
+			this(infos, warnings, errors, null, null, null);
 		}
 
 		/**
@@ -357,7 +367,7 @@ public abstract class AjcTestCase extends TestCase {
 		 * of any fail or abort messages in a CompilationResult will be a test failure. Informational messages will be ignored.
 		 */
 		public MessageSpec(List<AjcTestCase.Message> warnings, List<AjcTestCase.Message> errors) {
-			this(null, warnings, errors, null, null);
+			this(null, warnings, errors, null, null, null);
 		}
 	}
 
@@ -422,8 +432,8 @@ public abstract class AjcTestCase extends TestCase {
 	/**
 	 * Assert that no (non-informational) messages where produced during a compiler run.
 	 */
-	public void assertNoMessages(CompilationResult result, String message) {
-		assertMessages(result, message, MessageSpec.EMPTY_MESSAGE_SET);
+	public void assertNoMessages(CompilationResult result, String assertionFailedMessage) {
+		assertMessages(result, assertionFailedMessage, MessageSpec.EMPTY_MESSAGE_SET);
 	}
 
 	/**
@@ -436,7 +446,7 @@ public abstract class AjcTestCase extends TestCase {
 	/**
 	 * Assert that messages in accordance with the <code>expected</code> message specification where produced during a compiler run.
 	 */
-	public void assertMessages(CompilationResult result, String message, MessageSpec expected) {
+	public void assertMessages(CompilationResult result, String assertionFailedMessage, MessageSpec expected) {
 		if (result == null)
 			fail("Attempt to compare null compilation results against expected.");
 		List<AjcTestCase.Message> missingFails = copyAll(expected.fails);
@@ -457,10 +467,10 @@ public abstract class AjcTestCase extends TestCase {
 		}
 		compare(expected.weaves, result.getWeaveMessages(), missingWeaves, extraWeaves);
 
-		boolean infosEmpty = expected.isIgnoringInfoMessages() ? true : (missingInfos.isEmpty() && extraInfos.isEmpty());
+		boolean infosEmpty = expected.isIgnoringInfoMessages() || missingInfos.isEmpty() && extraInfos.isEmpty();
 		if (!(missingFails.isEmpty() && missingWarnings.isEmpty() && missingErrors.isEmpty() && missingWeaves.isEmpty()
 				&& extraFails.isEmpty() && extraWarnings.isEmpty() && extraErrors.isEmpty() && extraWeaves.isEmpty() && infosEmpty)) {
-			StringBuffer failureReport = new StringBuffer(message);
+			StringBuffer failureReport = new StringBuffer(assertionFailedMessage);
 			failureReport.append("\n");
 			if (!expected.isIgnoringInfoMessages()) {
 				addMissing(failureReport, "info", missingInfos);
@@ -484,7 +494,7 @@ public abstract class AjcTestCase extends TestCase {
 			}
 			String report = failureReport.toString();
 			System.err.println(failureReport);
-			fail(message + "'\n" + report);
+			fail(assertionFailedMessage + "'\n" + report);
 		}
 	}
 
diff --git a/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/MainTest.java b/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/MainTest.java
index ddcd1046a..f4b4bcd72 100644
--- a/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/MainTest.java
+++ b/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/MainTest.java
@@ -9,52 +9,70 @@
  * Contributors: 
  *     Wes Isberg     initial implementation 
  * ******************************************************************/
-
 package org.aspectj.tools.ajc;
 
+import org.aspectj.bridge.AbortException;
+
 import java.util.ArrayList;
 import java.util.List;
-import java.util.ResourceBundle;
-
-import org.aspectj.bridge.AbortException;
 
-/**
- * 
- */
 public class MainTest extends AjcTestCase {
 	
-    public void testMainbare() {
-        List<String> list = new ArrayList<>();
-// Usage now printed by Eclipse compiler so doesn't appear here in our message list
-//        Main.bareMain(new String[] {"-help"}, false, list, null, null, null);
-//        assertTrue(1 == list.size());
-        Main.bareMain(new String[] {"-X"}, false, list, null, null, null);
-        assertTrue(1 == list.size());        Object o = list.get(0);
-        assertTrue(o instanceof String);
-//        assertTrue(-1 != ((String)o).indexOf("-aspectpath"));
-//        assertTrue(-1 != ((String)o).indexOf("-incremental"));
+  public void testBareMainUsage() {
+    List<String> fails = new ArrayList<>();
+    List<String> errors = new ArrayList<>();
+    List<String> warnings = new ArrayList<>();
+    List<String> infos = new ArrayList<>();
+    List<String> usages = new ArrayList<>();
+    Main.bareMain(new String[] { "-?" }, false, fails, errors, warnings, infos, usages);
+    assertNotNull(
+      "usage text not found in compiler output",
+      usages.stream()
+        .filter(message -> message.contains("AspectJ-specific options:"))
+        .findFirst()
+        .orElse(null)
+    );
+  }
+
+  public void testBareMainUsageX() {
+    List<String> fails = new ArrayList<>();
+    List<String> errors = new ArrayList<>();
+    List<String> warnings = new ArrayList<>();
+    List<String> infos = new ArrayList<>();
+    List<String> usages = new ArrayList<>();
+    Main.bareMain(new String[] { "-X" }, false, fails, errors, warnings, infos, usages);
+    assertNotNull(
+      "usage text not found in compiler output",
+      usages.stream()
+        .filter(message -> message.contains("AspectJ-specific non-standard options:"))
+        .findFirst()
+        .orElse(null)
+    );
     }
     
-    public void testDashX() {
-    	String xoptionText = ResourceBundle.getBundle("org.aspectj.ajdt.ajc.messages").getString("xoption.usage");
-        xoptionText = "non-standard options:"; //xoptionText.substring("{0}".length());
-		CompilationResult result = ajc(null,new String[] {"-X"});
-		assertMessages(result,"Expecting xoptions usage message",
-				new MessageSpec(null,null,null,newMessageList(new Message(xoptionText)),null));
+  public void testAjcUsageX() {
+    CompilationResult compilationResult = ajc(null, new String[] { "-X" });
+    MessageSpec messageSpec = new MessageSpec(
+      null, null, null, null, null,
+      newMessageList(new Message("AspectJ-specific non-standard options:"))
+    );
+    assertMessages(compilationResult, "Expecting xoptions usage message", messageSpec);
     }
     
-    public void testDashMessageHolder() {
+  public void testMainMessageHolderFail() {
     	try {
     		new Main().runMain(new String[] {"-messageHolder","org.xyz.abc"},false);
-    		fail ("Should have thrown abort exception");
-    	} catch (AbortException ex) {
+      fail("ajc should have thrown abort exception");
+    }
+    catch (AbortException ex) {
     		// good
     	}
     }
     
-    public void testDashMessageHolderOk() {
+  public void testMainMessageHolderOk() {
     	Main main = new Main();
     	main.runMain(new String[] {"-messageHolder","org.aspectj.tools.ajc.TestMessageHolder"},false);
     	assertSame("ajc should be using our message handler",TestMessageHolder.class,main.getHolder().getClass());
     }
+
 }