Merge pull request #131 from google/fix-logger

Fixing defect where sharedtestsuites always log the first sentence re…

Author: incident-recipient

pom.xml

@@ -5,7 +5,7 @@
 
     <groupId>com.google.pdsl</groupId>
     <artifactId>pdsl</artifactId>
-    <version>1.10.0</version>
+    <version>1.10.1</version>
 
     <name>pdsl</name>
     <url>http://www.github.com/google/polymorphicDSL</url>

src/main/java/com/pdsl/executors/ColorizedLoggerObserver.java

@@ -4,6 +4,7 @@
 import com.pdsl.logging.PdslThreadSafeOutputStream;
 import com.pdsl.reports.MetadataTestRunResults;
 import com.pdsl.specifications.PolymorphicDslTransformationException;
+import com.pdsl.testcases.SharedTestCase;
 import com.pdsl.testcases.TestCase;
 import com.pdsl.testcases.TestSection;
 import org.antlr.v4.runtime.tree.ParseTreeListener;
@@ -14,6 +15,7 @@
 import java.io.InputStream;
 import java.nio.charset.Charset;
 import java.util.Collection;
+import java.util.Iterator;
 
 /**
  * A logger for the progress of test execution where the output has color.
@@ -26,6 +28,14 @@ public class ColorizedLoggerObserver implements ExecutorObserver {
     private static final String exceptionMessage = "Could not log!";
     private final Charset charset;
     private final byte[] RESET;
+    private Strategy strategy = Strategy.HIDE_INTERPRETER;
+    // Avoid race conditions in logic
+    // where a sharedtestsuite might be running with normal test suites in concurrent environment
+    private int concurrencyCounter = 0;
+    private enum Strategy {
+        LOG_INTERPRETER,
+        HIDE_INTERPRETER;
+    }
 
     public ColorizedLoggerObserver() {
         this.charset = Charset.defaultCharset();
@@ -61,11 +71,24 @@ private void notifyStreams(String str) {
         }
     }
 
+    @Override
+    public void onBeforeTestSuite(Collection<? extends SharedTestCase> testCases,
+                                   String context) {
+        strategy = Strategy.LOG_INTERPRETER;
+        logBeforeSuite();
+    }
+
     @Override
     public void onBeforeTestCase(TestCase testCase) {
         notifyStreams(AnsiTerminalColorHelper.YELLOW
                 + String.format("%s%n%s%n", testCase.getOriginalSource(), testCase.getTestTitle()
                 + AnsiTerminalColorHelper.RESET));
+       Object longDescription = testCase.getMetadata().get(TestCase.STANDARD_LONG_DESCRIPTION_KEY);
+        if (longDescription instanceof InputStream descriptionStream) {
+            notifyStreams(AnsiTerminalColorHelper.CYAN.getBytes(charset));
+            notifyStreams(descriptionStream);
+            notifyStreams(RESET);
+        }
     }
 
     @Override
@@ -75,27 +98,25 @@ public void onTestCaseSuccess(TestCase testCase) {
                         + AnsiTerminalColorHelper.RESET).getBytes(charset));
     }
 
-    private void beforePhrase(TestSection testSection) {
-        if (testSection.getMetaData().isPresent()) {
-            notifyStreams(AnsiTerminalColorHelper.CYAN.getBytes(charset));
-            notifyStreams(testSection.getMetaData().get());
-            notifyStreams(RESET);
+    private void notateInterpreter(Class<?> interpreter) {
+        if (strategy.equals(Strategy.LOG_INTERPRETER)) {
+            notifyStreams(AnsiTerminalColorHelper.GREY);
+            notifyStreams(String.format("^ %s %n", interpreter));
+            notifyStreams(AnsiTerminalColorHelper.RESET);
         }
     }
 
-    @Override
-    public void onBeforePhrase(ParseTreeVisitor<?> visitor, TestSection testSection) {
-        beforePhrase(testSection);
-    }
-
     @Override
     public void onAfterPhrase(ParseTreeListener listener, ParseTreeWalker walker, TestSection testSection) {
         afterPhrase(testSection);
+        notateInterpreter(listener.getClass());
     }
 
     @Override
     public void onAfterPhrase(ParseTreeVisitor<?> visitor, TestSection testSection) {
+
         afterPhrase(testSection);
+        notateInterpreter(visitor.getClass());
     }
 
     private void afterPhrase(TestSection testSection) {
@@ -138,6 +159,10 @@ public void onAfterTestSuite(Collection<? extends TestCase> testCases, ParseTree
     @Override
     public void onAfterTestSuite(Collection<? extends TestCase> testCases, ParseTreeListener listener, MetadataTestRunResults results, String context) {
         logAfterSuite(results);
+        concurrencyCounter--;
+        if (concurrencyCounter <= 0) {
+            strategy = Strategy.HIDE_INTERPRETER;
+        }
     }
 
     @Override
@@ -159,5 +184,6 @@ private  void logAfterSuite(MetadataTestRunResults results) {
 
     private void logBeforeSuite() {
         notifyStreams(AnsiTerminalColorHelper.BRIGHT_YELLOW + "Running tests..." + AnsiTerminalColorHelper.RESET);
+        concurrencyCounter++;
     }
 }

src/main/java/com/pdsl/executors/DefaultPolymorphicDslTestExecutor.java

@@ -20,6 +20,7 @@
 
 import java.util.*;
 import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.stream.Collectors;
 
 /**
  * An executor that runs PDSL tests create from a TestCaseFactory.
@@ -191,7 +192,7 @@ public MetadataTestRunResults runTestsWithMetadata(Collection<SharedTestCase> sh
             List<TestCase> listOfTestCases = sharedTestCase.getSharedTestCaseWithInterpreters().stream()
                     .map(SharedTestCaseWithInterpreter::getTestCase).toList();
             int size = listOfTestCases.getFirst().getUnfilteredPhraseBody().size();
-            TestCase testCase = listOfTestCases.stream().findFirst().orElseThrow();
+
             notifyBeforeTestCase(sharedTestCase);
             // Create each visitor/listener one time per test case
             Map<InterpreterObj, ParseTreeListener> suppliedListeneres = new HashMap<>();
@@ -209,6 +210,11 @@ public MetadataTestRunResults runTestsWithMetadata(Collection<SharedTestCase> sh
                     .map(Iterator::next)
                     .findFirst()
                     .orElseThrow(() -> new IllegalStateException("No executable phrases were found!"));
+            Map<SharedTestCaseWithInterpreter, Iterator<TestSection>> interpreter2Iterator = sharedTestCase.getSharedTestCaseWithInterpreters()
+                    .stream()
+                    .collect(Collectors.toMap(interpreter -> interpreter,
+                            interpreter -> interpreter.getTestCase().getContextFilteredTestSectionIterator()
+                    ));
 
             try {
                 for (int j = 0;
@@ -221,8 +227,13 @@ public MetadataTestRunResults runTestsWithMetadata(Collection<SharedTestCase> sh
                         //TODO - Add implementation for the duplication checking
                         Optional<ParseTree> parseTree = filteredPhrase.getParseTree();
                         if (parseTree.isPresent()) {
+                            if (!interpreter2Iterator.get(interpreter).hasNext()) {
+                                // If a parse tree was found there should definitely be a new test section or something
+                                // is wrong
+                                throw new IllegalStateException("PDSL Framework error: The context filtered interpreters are out of sync!");
+                            }
+                            testSection = interpreter2Iterator.get(interpreter).next();
                             phrase = Optional.of(new DefaultPhrase(parseTree.get(), phraseIndex));
-
                             interpreterObj = Optional.of(interpreter.getInterpreterObj());
 
                             if (interpreterObj.get().getListenerSupplier().isPresent()) {
@@ -241,27 +252,26 @@ public MetadataTestRunResults runTestsWithMetadata(Collection<SharedTestCase> sh
                                 visitor.visit(parseTree.get());
                                 notifyAfterVisitor(visitor, testSection);
                             }
-
                         }
                     }
                     phraseIndex++;
                 }
-                notifyTestCaseSuccess(testCase);
+                notifyTestCaseSuccess(sharedTestCase);
             } catch (Throwable e) {
                 if (interpreterObj.isPresent() && phrase.isPresent()) {
                     if (interpreterObj.get().getParseTreeListener().isPresent()) {
                         notifyOnListenerException(interpreterObj.get().getParseTreeListener().get(),
-                                testSection, testCase, e);
+                                testSection, sharedTestCase, e);
                     } else if (interpreterObj.get().getParseTreeVisitor().isPresent()) {
                         notifyOnVisitorException(interpreterObj.get().getParseTreeVisitor().get(),
-                                testSection, testCase, e);
+                                testSection, sharedTestCase, e);
                     }
                 }
-                results.addTestResult(DefaultTestResult.failedTest(testCase, null, e, phraseIndex,
+                results.addTestResult(DefaultTestResult.failedTest(sharedTestCase, null, e, phraseIndex,
                         size - phraseIndex));
             }
-            results.addTestResult(DefaultTestResult.passingTest(testCase));
-            notifyAfterTestCase(testCase);
+            results.addTestResult(DefaultTestResult.passingTest(sharedTestCase));
+            notifyAfterTestCase(sharedTestCase);
         }
         notifyAfterTestSuite(sharedTestCases, results, context);
         return results;
@@ -286,7 +296,7 @@ public MetadataTestRunResults runTestsWithMetadata(Collection<SharedTestCase> sh
      *                           be called
      * testCaseSuccess         - Called once after each test case only if it had no
      *                           phrase failures
-     * @param observer
+     * @param observer an observer to notify during test execution events
      */
     @Override
     public void registerObserver(ExecutorObserver observer) {

src/main/java/com/pdsl/testcases/DefaultPdslTestCase.java

@@ -8,6 +8,7 @@
 import org.antlr.v4.runtime.tree.ParseTree;
 
 import java.util.*;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -22,6 +23,8 @@ public class DefaultPdslTestCase implements TestCase {
     private final List<String> contextFilteredPhraseBody;
     private final List<FilteredPhrase> phrasesToTestSections;
     private final URI source;
+    private final Map<String, Object> metadata;
+    private static final String errMessage = "Test case title cannot be empty or null!";
 
     public static final PdslTestCaseComparator DEFAULT_TEST_CASE_COMPARATOR = new PdslTestCaseComparator();
 
@@ -33,7 +36,7 @@ public class DefaultPdslTestCase implements TestCase {
      * @param source the original source this test case was created from
      */
     public DefaultPdslTestCase(String testCaseTitle, List<TestBodyFragment> testBodyFragments, URI source) {
-        String errMessage = "Test case title cannot be empty or null!";
+
         Preconditions.checkNotNull(testCaseTitle, errMessage);
         Preconditions.checkNotNull(source);
         Preconditions.checkArgument(!testCaseTitle.isEmpty(), errMessage);
@@ -55,6 +58,39 @@ public DefaultPdslTestCase(String testCaseTitle, List<TestBodyFragment> testBody
                 .map(Optional::get)
                 .map(ParseTree::getText)
                 .toList();
+       metadata = new ConcurrentHashMap<>();
+    }
+
+    public DefaultPdslTestCase(String testCaseTitle, List<TestBodyFragment> testBodyFragments, URI source,
+                               Map<String, Object> metadata) {
+        Preconditions.checkNotNull(testCaseTitle, errMessage);
+        Preconditions.checkNotNull(source);
+        Preconditions.checkArgument(!testCaseTitle.isEmpty(), errMessage);
+        Preconditions.checkNotNull(testBodyFragments, errMessage);
+        Preconditions.checkArgument(!testBodyFragments.isEmpty(), errMessage);
+        Preconditions.checkNotNull(metadata);
+        this.source = source;
+        this.testBodyFragments = testBodyFragments;
+        this.testCaseTitle = testCaseTitle;
+        this.phrasesToTestSections = testBodyFragments.stream()
+                .map(TestBodyFragment::getTestPhrases)
+                .flatMap(Collection::stream)
+                .toList();
+        this.unfilteredPhraseBody = phrasesToTestSections.stream()
+                .map(FilteredPhrase::getPhrase)
+                .toList();
+        this.contextFilteredPhraseBody = phrasesToTestSections.stream()
+                .map(FilteredPhrase::getParseTree)
+                .filter(Optional::isPresent)
+                .map(Optional::get)
+                .map(ParseTree::getText)
+                .toList();
+        this.metadata = metadata;
+    }
+
+    @Override
+    public Map<String, Object> getMetadata() {
+        return metadata;
     }
 
     @Override

src/main/java/com/pdsl/testcases/DefaultTaggedTestCase.java

@@ -6,21 +6,22 @@
 import java.util.Collection;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 
 /** A test case that has arbitrary tags associated with it. */
 public class DefaultTaggedTestCase implements TaggedTestCase {
 
     private final TestCase testCase;
-    private final Collection<String> tags;
-
+    private final Collection<String> tags; // Keep to avoid the need for casting and checking generic types
     public DefaultTaggedTestCase(TestCase testCase, Collection<String> tags) {
         this.testCase = testCase;
         this.tags = tags;
+        testCase.getMetadata().put(TestCase.DEFAULT_TAGS, this.tags); // Add a reference just to be thorough
     }
 
     @Override
     public Collection<String> getTags() {
-        return tags;
+       return tags;
     }
 
     @Override
@@ -52,4 +53,9 @@ public Iterator<TestSection> getContextFilteredTestSectionIterator() {
     public List<FilteredPhrase> getFilteredPhrases() {
         return testCase.getFilteredPhrases();
     }
+
+    @Override
+    public Map<String, Object> getMetadata() {
+        return testCase.getMetadata();
+    }
 }

src/main/java/com/pdsl/testcases/DefaultTestSection.java

@@ -3,7 +3,9 @@
 import com.pdsl.specifications.Phrase;
 
 import java.io.InputStream;
+import java.util.Map;
 import java.util.Optional;
+import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * A container of phrases representing a chunk of a test case.
@@ -13,18 +15,22 @@
  */
 public class DefaultTestSection implements TestSection {
 
-    private final Optional<InputStream> metaData;
-    private final Phrase phrase;
 
+    private final Phrase phrase;
+    private final Optional<Map<String, Object>> metadata;
+    private final Optional<InputStream> longDescription;
     /**
      * Creates a test section with the provided phrase.
      *
-     * @param metaData additional information about the phrase
+     * @param longDescription additional information about the phrase
      * @param phrase an arbitrary sentence recognized by a parser
      */
-    public DefaultTestSection(InputStream metaData, Phrase phrase) {
-        this.metaData = Optional.ofNullable(metaData);
+    public DefaultTestSection(InputStream longDescription, Phrase phrase) {
+        Map<String, Object> metadataMap = new ConcurrentHashMap<>(1);
+        metadataMap.put(TestCase.STANDARD_LONG_DESCRIPTION_KEY, longDescription);
+        this.metadata = Optional.of(metadataMap);
         this.phrase = phrase;
+        this.longDescription = Optional.ofNullable(longDescription);
     }
 
     /**
@@ -33,13 +39,19 @@ public DefaultTestSection(InputStream metaData, Phrase phrase) {
      * @param phrase an arbitrary sentence recognized by a parser
      */
     public DefaultTestSection(Phrase phrase) {
-        this.metaData = Optional.empty();
+        this.metadata = Optional.empty();
         this.phrase = phrase;
+        this.longDescription = Optional.empty();
     }
 
     @Override
     public Optional<InputStream> getMetaData() {
-        return metaData;
+        return longDescription;
+    }
+
+    @Override
+    public Optional<Map<String, Object>> getSectionMetadata() {
+        return metadata;
     }
 
     @Override

src/main/java/com/pdsl/testcases/PreorderTestCaseFactory.java

@@ -64,7 +64,7 @@ private List<TestCase> recursiveWalkAndCreateOnLeaf(TestSpecification testSpecif
         }
         // Add phrases in this node if present
         if (testSpecification.getFilteredPhrases().isPresent()) {
-            childTestBodyFragments.add(new TestBodyFragment(childMetaData.isPresent() ? childMetaData.get() : null, testSpecification.getFilteredPhrases().get()));
+            childTestBodyFragments.add(new TestBodyFragment(null, testSpecification.getFilteredPhrases().get()));
         }
         // Add phrases in child node if present
         if (testSpecification.nestedTestSpecifications().isPresent()) {
@@ -74,6 +74,7 @@ private List<TestCase> recursiveWalkAndCreateOnLeaf(TestSpecification testSpecif
             return testCases;
         } else {
             TestCase testCase = new DefaultPdslTestCase(testSpecification.getName(), childTestBodyFragments, testSpecification.getOriginalTestResource());
+            testCase.getMetadata().put(TestCase.STANDARD_LONG_DESCRIPTION_KEY, childMetaData);
             if (!tags.isEmpty()) {
                 testCase = new DefaultTaggedTestCase(testCase, tags);
             }