From 1dea8dc3bb66c23abf4a4c9329bd024e445433fd Mon Sep 17 00:00:00 2001 From: Roman Ivanov Date: Wed, 14 Dec 2016 18:03:40 -0800 Subject: [PATCH] Issue #3657: skip unnecessary exception 'Severity not set, ignoring exception' --- .ci/travis/travis.sh | 3 +- .../tools/checkstyle/ConfigurationLoader.java | 40 ++++++++++++------ .../checkstyle/ConfigurationLoaderTest.java | 42 +++++++++++++++++++ .../checkstyle/DefaultConfigurationTest.java | 18 ++++++++ 4 files changed, 89 insertions(+), 14 deletions(-) diff --git a/.ci/travis/travis.sh b/.ci/travis/travis.sh index bb91a0f99..112f8ebc5 100755 --- a/.ci/travis/travis.sh +++ b/.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 diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/ConfigurationLoader.java b/src/main/java/com/puppycrawl/tools/checkstyle/ConfigurationLoader.java index 8740a6f17..eccf2192a 100644 --- a/src/main/java/com/puppycrawl/tools/checkstyle/ConfigurationLoader.java +++ b/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 result = Arrays.stream(names) + .filter(name -> name.equals(attributeName)).findFirst(); + return result.isPresent(); + } } } diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/ConfigurationLoaderTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/ConfigurationLoaderTest.java index c8aa29976..765818e39 100644 --- a/src/test/java/com/puppycrawl/tools/checkstyle/ConfigurationLoaderTest.java +++ b/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()); + } + } + } diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/DefaultConfigurationTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/DefaultConfigurationTest.java index 02f535ad5..53630390d 100644 --- a/src/test/java/com/puppycrawl/tools/checkstyle/DefaultConfigurationTest.java +++ b/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()); + } + } }