This is an automated email from the ASF dual-hosted git repository. vladimirsitnikov pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/jmeter.git
commit 1e45414004b0ebbe9891d4ec3468d8c1fb2a7ef8 Author: Vladimir Sitnikov <[email protected]> AuthorDate: Tue Jun 27 19:17:45 2023 +0300 test: add test that verifies test element does not change after save+load roundtrip ProxyControl initialized collection properties with HashSet which caused CollectionProperty.equals to return false. CollectionProperty is normalized to ArrayList on save, so we should not pass HashSet to the CollectionProperty in the first place. --- .../jmeter/testelement/AbstractTestElement.java | 14 ++-- .../org/apache/jmeter/dsl/DslPrinterTraverser.kt | 2 +- .../java/org/apache/jmeter/junit/JMeterTest.java | 78 ++++++++++++++++++++++ .../org/apache/jmeter/save/TestSaveService.java | 2 +- .../jmeter/protocol/http/proxy/ProxyControl.java | 13 +++- 5 files changed, 101 insertions(+), 8 deletions(-) diff --git a/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java b/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java index fc885f89c2..0486721475 100644 --- a/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java +++ b/src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java @@ -482,10 +482,16 @@ public abstract class AbstractTestElement implements TestElement, Serializable, } else { writeLock(); try { - propMap.put(property.getName(), property); - Map<String, JMeterProperty> propMapConcurrent = this.propMapConcurrent; - if (propMapConcurrent != null) { - propMapConcurrent.put(property.getName(), property); + if (property instanceof StringProperty && property.getStringValue() == null) { + // Avoid storing properties with null values since they will be skipped anyway when saving + // the test plan + removeProperty(property.getName()); + } else { + propMap.put(property.getName(), property); + Map<String, JMeterProperty> propMapConcurrent = this.propMapConcurrent; + if (propMapConcurrent != null) { + propMapConcurrent.put(property.getName(), property); + } } } finally { writeUnlock(); diff --git a/src/core/src/main/kotlin/org/apache/jmeter/dsl/DslPrinterTraverser.kt b/src/core/src/main/kotlin/org/apache/jmeter/dsl/DslPrinterTraverser.kt index 81702d19ab..a17022298c 100644 --- a/src/core/src/main/kotlin/org/apache/jmeter/dsl/DslPrinterTraverser.kt +++ b/src/core/src/main/kotlin/org/apache/jmeter/dsl/DslPrinterTraverser.kt @@ -173,7 +173,7 @@ public class DslPrinterTraverser( is DoubleProperty -> append(property.stringValue) is FloatProperty -> append(property.stringValue).append('f') is LongProperty -> append(property.stringValue).append('d') - is StringProperty -> appendLiteral(property.stringValue) + is StringProperty -> property.stringValue?.let { appendLiteral(it) } ?: run { append("null") } is TestElementProperty -> { val element = property.element diff --git a/src/dist-check/src/test/java/org/apache/jmeter/junit/JMeterTest.java b/src/dist-check/src/test/java/org/apache/jmeter/junit/JMeterTest.java index 19152c90ab..f7463cbcd9 100644 --- a/src/dist-check/src/test/java/org/apache/jmeter/junit/JMeterTest.java +++ b/src/dist-check/src/test/java/org/apache/jmeter/junit/JMeterTest.java @@ -28,6 +28,7 @@ import java.io.ObjectOutputStream; import java.io.Serializable; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Modifier; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; @@ -35,6 +36,7 @@ import java.util.Iterator; import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Objects; import java.util.Properties; import java.util.stream.Collectors; @@ -45,6 +47,7 @@ import javax.xml.parsers.ParserConfigurationException; import org.apache.commons.lang3.StringUtils; import org.apache.jmeter.config.gui.ObsoleteGui; +import org.apache.jmeter.dsl.DslPrinterTraverser; import org.apache.jmeter.gui.JMeterGUIComponent; import org.apache.jmeter.gui.UnsharedComponent; import org.apache.jmeter.gui.tree.JMeterTreeNode; @@ -52,6 +55,8 @@ import org.apache.jmeter.save.SaveService; import org.apache.jmeter.testbeans.TestBean; import org.apache.jmeter.testbeans.gui.TestBeanGUI; import org.apache.jmeter.testelement.TestElement; +import org.apache.jmeter.testelement.property.JMeterProperty; +import org.apache.jmeter.testelement.property.PropertyIterator; import org.apache.jmeter.util.JMeterUtils; import org.apache.jorphan.reflect.ClassFinder; import org.apache.jorphan.util.JOrphanUtils; @@ -270,6 +275,8 @@ public class JMeterTest extends JMeterTestCaseJUnit implements Describable { System.out.println("o.a.j.junit.JMeterTest INFO: JMeterGUIComponent: skipping some tests " + item.getClass().getName()); } else { ts.addTest(new JMeterTest("GUIComponents2", item)); + ts.addTest(new JMeterTest("saveLoadShouldKeepElementIntact", item)); + ts.addTest(new JMeterTest("propertiesShouldNotBeInitializedToNullValues", item)); ts.addTest(new JMeterTest("runGUITitle", item)); } suite.addTest(ts); @@ -289,6 +296,8 @@ public class JMeterTest extends JMeterTestCaseJUnit implements Describable { JMeterGUIComponent item = new TestBeanGUI(c); TestSuite ts = new TestSuite(item.getClass().getName()); ts.addTest(new JMeterTest("GUIComponents2", item)); + ts.addTest(new JMeterTest("saveLoadShouldKeepElementIntact", item)); + ts.addTest(new JMeterTest("propertiesShouldNotBeInitializedToNullValues", item)); ts.addTest(new JMeterTest("runGUITitle", item)); suite.addTest(ts); } catch (IllegalArgumentException e) { @@ -386,6 +395,75 @@ public class JMeterTest extends JMeterTestCaseJUnit implements Describable { assertEquals("Modify Test: Failed on " + name, "hey, new name!:", el2.getName()); } + public void propertiesShouldNotBeInitializedToNullValues() { + TestElement el = guiItem.createTestElement(); + PropertyIterator it = el.propertyIterator(); + while (it.hasNext()) { + JMeterProperty property = it.next(); + if (property.getObjectValue() == null) { + fail( + "Property " + property.getName() + " is initialized with NULL OBJECT value in " + + " test element " + el + " created with " + guiItem + ".createTestElement() " + + "Please refrain from that since null properties consume memory, and they will be " + + "removed when saving and loading the plan anyway" + ); + } + if (property.getStringValue() == null) { + fail( + "Property " + property.getName() + " is initialized with NULL STRING value in " + + " test element " + el + " created with " + guiItem + ".createTestElement() " + + "Please refrain from that since null properties consume memory, and they will be " + + "removed when saving and loading the plan anyway" + ); + } + } + } + + public void saveLoadShouldKeepElementIntact() throws IOException { + TestElement expected = guiItem.createTestElement(); + ByteArrayOutputStream bos = new ByteArrayOutputStream(); + SaveService.saveElement(expected, bos); + byte[] serializedBytes = bos.toByteArray(); + TestElement actual = (TestElement) SaveService.loadElement(new ByteArrayInputStream(serializedBytes)); + compareAllProperties(expected, actual, serializedBytes); + } + + private static void compareAllProperties(TestElement expected, TestElement actual, byte[] serializedBytes) { + // JMeter restores "enabled" as StringProperty, see + // org.apache.jmeter.save.converters.ConversionHelp.restoreSpecialProperties + // So we normalize it back to BooleanProperty + JMeterProperty expEnabled = expected.getPropertyOrNull(expected.getSchema().getEnabled()); + if (expEnabled != null && (expEnabled.getStringValue().equals("true") || expEnabled.getStringValue().equals("false"))) { + JMeterProperty actEnabled = actual.getPropertyOrNull(actual.getSchema().getEnabled()); + if (actEnabled != null && actEnabled.getStringValue().equals(expEnabled.getStringValue())) { + actual.setProperty(expEnabled); + } + } + + String expectedStr = new DslPrinterTraverser(DslPrinterTraverser.DetailLevel.ALL).append(expected).toString(); + if (!Objects.equals(expected, actual)) { + boolean abc = Objects.equals(expected, actual); + assertEquals( + "TestElement after 'save+load' should match the one created in GUI\n" + + "JMX is " + new String(serializedBytes, StandardCharsets.UTF_8), + expectedStr, + new DslPrinterTraverser(DslPrinterTraverser.DetailLevel.ALL).append(actual).toString() + ); + fail("TestElement after 'save+load' should match the one created in GUI. " + + "DSL representation is the same, however TestElement#equals says the elements are different. " + + "DSL is " + expectedStr + "\n" + + "JMX is " + new String(serializedBytes, StandardCharsets.UTF_8)); + } + assertEquals( + "TestElement.hashCode after 'save+load' should match the one created in GUI. " + + "DSL representation is the same, however TestElement#hashCode says the elements are different. " + + "DSL is " + expectedStr + "\n" + + "JMX is " + new String(serializedBytes, StandardCharsets.UTF_8), + expected.hashCode(), + actual.hashCode() + ); + } + /* * Test serializable elements - create the suite of tests */ diff --git a/src/dist-check/src/test/java/org/apache/jmeter/save/TestSaveService.java b/src/dist-check/src/test/java/org/apache/jmeter/save/TestSaveService.java index 9b44acc7d7..6a64173910 100644 --- a/src/dist-check/src/test/java/org/apache/jmeter/save/TestSaveService.java +++ b/src/dist-check/src/test/java/org/apache/jmeter/save/TestSaveService.java @@ -117,7 +117,7 @@ public class TestSaveService extends JMeterTestCase { final FileStats origStats = getFileStats(testFile); final FileStats savedStats = getFileStats(savedFile); - ByteArrayOutputStream out = new ByteArrayOutputStream(1000000); + ByteArrayOutputStream out = new ByteArrayOutputStream(Math.toIntExact(testFile.length())); try { HashTree tree = SaveService.loadTree(testFile); SaveService.saveTree(tree, out); diff --git a/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/ProxyControl.java b/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/ProxyControl.java index 9b5a6623c9..01155e769a 100644 --- a/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/ProxyControl.java +++ b/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/ProxyControl.java @@ -45,6 +45,7 @@ import java.util.Map; import java.util.Set; import java.util.prefs.Preferences; import java.util.regex.PatternSyntaxException; +import java.util.stream.Collectors; import org.apache.commons.codec.binary.Base64; import org.apache.commons.codec.digest.DigestUtils; @@ -398,11 +399,19 @@ public class ProxyControl extends GenericController implements NonTestElement { } public void setIncludeList(Collection<String> list) { - setProperty(new CollectionProperty(INCLUDE_LIST, new HashSet<>(list))); + if (list.size() >= 2) { + // Deduplicate if there is more than one element in the list + list = list.stream().distinct().collect(Collectors.toList()); + } + setProperty(new CollectionProperty(INCLUDE_LIST, list)); } public void setExcludeList(Collection<String> list) { - setProperty(new CollectionProperty(EXCLUDE_LIST, new HashSet<>(list))); + if (list.size() >= 2) { + // Deduplicate if there is more than one element in the list + list = list.stream().distinct().collect(Collectors.toList()); + } + setProperty(new CollectionProperty(EXCLUDE_LIST, list)); } public void setRegexMatch(boolean b) {
