Issue #3657: skip unnecessary exception 'Severity not set, ignoring exception'

.ci/travis/travis.sh

@@ -5,7 +5,8 @@ set -e
 case "$GOAL" in
 
 nondex)
-  mvn --fail-never clean nondex:nondex
+  # exclude ConfigurationLoaderTest till https://github.com/TestingResearchIllinois/NonDex/issues/112
+  mvn --fail-never clean nondex:nondex -Dtest='*,!ConfigurationLoaderTest'
   cat `grep -RlE 'td class=.x' .nondex/ | cat` < /dev/null > output.txt
   RESULT=$(cat output.txt | wc -c)
   cat output.txt

src/main/java/com/puppycrawl/tools/checkstyle/ConfigurationLoader.java

@@ -24,17 +24,17 @@ import java.io.InputStream;
 import java.net.URI;
 import java.util.ArrayDeque;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Deque;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Optional;
 
 import javax.xml.parsers.ParserConfigurationException;
 
-import org.apache.commons.logging.Log;
-import org.apache.commons.logging.LogFactory;
 import org.xml.sax.Attributes;
 import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
@@ -52,8 +52,6 @@ import com.puppycrawl.tools.checkstyle.utils.CommonUtils;
  * @author Oliver Burn
  */
 public final class ConfigurationLoader {
-    /** Logger for ConfigurationLoader. */
-    private static final Log LOG = LogFactory.getLog(ConfigurationLoader.class);
 
     /** The public ID for version 1_0 of the configuration dtd. */
     private static final String DTD_PUBLIC_ID_1_0 =
@@ -466,21 +464,24 @@ public final class ConfigurationLoader {
         @Override
         public void endElement(String uri,
                                String localName,
-                               String qName) {
+                               String qName) throws SAXException {
             if (qName.equals(MODULE)) {
 
                 final Configuration recentModule =
                     configStack.pop();
 
-                // remove modules with severity ignore if these modules should
-                // be omitted
+                // get severity attribute if it exists
                 SeverityLevel level = null;
-                try {
-                    final String severity = recentModule.getAttribute(SEVERITY);
-                    level = SeverityLevel.getInstance(severity);
-                }
-                catch (final CheckstyleException ex) {
-                    LOG.debug("Severity not set, ignoring exception", ex);
+                if (containsAttribute(recentModule, SEVERITY)) {
+                    try {
+                        final String severity = recentModule.getAttribute(SEVERITY);
+                        level = SeverityLevel.getInstance(severity);
+                    }
+                    catch (final CheckstyleException ex) {
+                        throw new SAXException(
+                                "Problem during accessing '" + SEVERITY + "' attribute for "
+                                        + recentModule.getName(), ex);
+                    }
                 }
 
                 // omit this module if these should be omitted and the module
@@ -495,5 +496,18 @@ public final class ConfigurationLoader {
                 }
             }
         }
+
+        /**
+         * Util method to recheck attribute in module.
+         * @param module module to check
+         * @param attributeName name of attribute in module to find
+         * @return true if attribute is present in module
+         */
+        private boolean containsAttribute(Configuration module, String attributeName) {
+            final String[] names = module.getAttributeNames();
+            final Optional<String> result = Arrays.stream(names)
+                    .filter(name -> name.equals(attributeName)).findFirst();
+            return result.isPresent();
+        }
     }
 }

src/test/java/com/puppycrawl/tools/checkstyle/ConfigurationLoaderTest.java

@@ -22,6 +22,7 @@ package com.puppycrawl.tools.checkstyle;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.powermock.api.mockito.PowerMockito.when;
 
 import java.io.File;
 import java.io.FileInputStream;
@@ -32,6 +33,10 @@ import java.lang.reflect.Method;
 import java.util.Properties;
 
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
 import org.xml.sax.Attributes;
 import org.xml.sax.InputSource;
 
@@ -43,6 +48,8 @@ import com.puppycrawl.tools.checkstyle.api.Configuration;
  * @author Rick Giles
  * @author lkuehne
  */
+@RunWith(PowerMockRunner.class)
+@PrepareForTest({DefaultConfiguration.class, ConfigurationLoader.class})
 public class ConfigurationLoaderTest {
     private static String getConfigPath(String filename) {
         return "src/test/resources/com/puppycrawl/tools/checkstyle/configs/" + filename;
@@ -501,4 +508,39 @@ public class ConfigurationLoaderTest {
             fail("unexpected exception");
         }
     }
+
+    /**
+     * This SuppressWarning("unchecked") required to suppress
+     * "Unchecked generics array creation for varargs parameter" during mock
+     * @throws Exception could happen from PowerMokito calls and getAttribute
+     */
+    @SuppressWarnings("unchecked")
+    @Test
+    public void testConfigWithIgnoreExceptionalAttributes() throws Exception {
+
+        // emulate exception from unrelated code, but that is same try-catch
+        final DefaultConfiguration tested = PowerMockito.mock(DefaultConfiguration.class);
+        when(tested.getAttributeNames()).thenReturn(new String[] {"severity"});
+        when(tested.getName()).thenReturn("MemberName");
+        when(tested.getAttribute("severity")).thenThrow(CheckstyleException.class);
+        // to void creation of 2 other mocks for now reason, only one moc is used for all cases
+        PowerMockito.whenNew(DefaultConfiguration.class)
+                .withArguments("MemberName").thenReturn(tested);
+        PowerMockito.whenNew(DefaultConfiguration.class)
+                .withArguments("Checker").thenReturn(tested);
+        PowerMockito.whenNew(DefaultConfiguration.class)
+                .withArguments("TreeWalker").thenReturn(tested);
+
+        try {
+            ConfigurationLoader.loadConfiguration(
+                    getConfigPath("config_with_ignore.xml"),
+                    new PropertiesExpander(new Properties()), true);
+            fail("Exception is expected");
+        }
+        catch (CheckstyleException expected) {
+            assertEquals("Problem during accessing 'severity' attribute for MemberName",
+                    expected.getCause().getMessage());
+        }
+    }
+
 }

src/test/java/com/puppycrawl/tools/checkstyle/DefaultConfigurationTest.java

@@ -20,9 +20,12 @@
 package com.puppycrawl.tools.checkstyle;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
 
 import org.junit.Test;
 
+import com.puppycrawl.tools.checkstyle.api.CheckstyleException;
+
 public class DefaultConfigurationTest {
 
     @Test
@@ -35,4 +38,19 @@ public class DefaultConfigurationTest {
         config.removeChild(configChild);
         assertEquals(0, config.getChildren().length);
     }
+
+    @Test
+    public void testExceptioForNonExistingAttribute() {
+        final String name = "MyConfig";
+        final DefaultConfiguration config = new DefaultConfiguration(name);
+        final String attributeName = "NonExisting#$%";
+        try {
+            config.getAttribute(attributeName);
+            fail("Exception is expected");
+        }
+        catch (CheckstyleException expected) {
+            assertEquals("missing key '" + attributeName + "' in " + name,
+                    expected.getMessage());
+        }
+    }
 }