diff --git a/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java b/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java index 3105d0cf1ec..4080eae7764 100644 --- a/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java +++ b/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java @@ -730,15 +730,20 @@ public boolean hasArguments() { @Override public void addTestElement(TestElement el) { if (el instanceof CookieManager cookieManager) { - setCookieManager(cookieManager); + // Route through the private property setter to skip the "superseded by" warning: + // addTestElement is invoked on every thread-group iteration by TestCompiler, + // and the config element is the same instance as the one already attached, + // so the warning would always fire spuriously. Users still get the warning + // when they explicitly call setCookieManager with a different instance. + setCookieManagerProperty(cookieManager); } else if (el instanceof CacheManager cacheManager) { - setCacheManager(cacheManager); + setCacheManagerProperty(cacheManager); } else if (el instanceof HeaderManager headerManager) { setHeaderManager(headerManager); } else if (el instanceof AuthManager authManager) { - setAuthManager(authManager); + setAuthManagerProperty(authManager); } else if (el instanceof DNSCacheManager dnsCacheManager) { - setDNSResolver(dnsCacheManager); + setDNSResolverProperty(dnsCacheManager); } else if (el instanceof KeystoreConfig keystoreConfig) { setKeystoreConfigProperty(keystoreConfig); } else { @@ -923,13 +928,19 @@ public boolean getPostBodyRaw() { return get(getSchema().getPostBodyRaw()); } + @SuppressWarnings("ReferenceEquality") public void setAuthManager(AuthManager value) { AuthManager mgr = getAuthManager(); - if (mgr != null) { + if (mgr != null && mgr != value) { if(log.isWarnEnabled()) { log.warn("Existing AuthManager {} superseded by {}", mgr.getName(), value.getName()); } } + setAuthManagerProperty(value); + } + + // private method to allow AsyncSample and addTestElement to reset the value without performing checks + private void setAuthManagerProperty(AuthManager value) { set(getSchema().getAuthManager(), value); } @@ -961,9 +972,10 @@ private void setCookieManagerProperty(CookieManager value) { set(getSchema().getCookieManager(), value);; } + @SuppressWarnings("ReferenceEquality") public void setCookieManager(CookieManager value) { CookieManager mgr = getCookieManager(); - if (mgr != null) { + if (mgr != null && mgr != value) { if(log.isWarnEnabled()) { log.warn("Existing CookieManager {} superseded by {}", mgr.getName(), value.getName()); } @@ -983,9 +995,10 @@ private void setKeystoreConfigProperty(KeystoreConfig value) { set(getSchema().getKeystoreConfig(), value); } + @SuppressWarnings("ReferenceEquality") public void setKeystoreConfig(KeystoreConfig value) { KeystoreConfig mgr = getKeystoreConfig(); - if (mgr != null && log.isWarnEnabled()) { + if (mgr != null && mgr != value && log.isWarnEnabled()) { log.warn("Existing KeystoreConfig {} superseded by {}", mgr.getName(), value.getName()); } setKeystoreConfigProperty(value); @@ -995,9 +1008,10 @@ public KeystoreConfig getKeystoreConfig() { return getOrNull(getSchema().getKeystoreConfig()); } + @SuppressWarnings("ReferenceEquality") public void setCacheManager(CacheManager value) { CacheManager mgr = getCacheManager(); - if (mgr != null) { + if (mgr != null && mgr != value) { if(log.isWarnEnabled()) { log.warn("Existing CacheManager {} superseded by {}", mgr.getName(), value.getName()); } @@ -1013,13 +1027,19 @@ public DNSCacheManager getDNSResolver() { return getOrNull(getSchema().getDnsCacheManager()); } + @SuppressWarnings("ReferenceEquality") public void setDNSResolver(DNSCacheManager cacheManager) { DNSCacheManager mgr = getDNSResolver(); - if (mgr != null) { + if (mgr != null && mgr != cacheManager) { if(log.isWarnEnabled()) { log.warn("Existing DNSCacheManager {} superseded by {}", mgr.getName(), cacheManager.getName()); } } + setDNSResolverProperty(cacheManager); + } + + // private method to allow addTestElement to reset the value without performing checks + private void setDNSResolverProperty(DNSCacheManager cacheManager) { set(getSchema().getDnsCacheManager(), cacheManager); } diff --git a/src/protocol/http/src/test/java/org/apache/jmeter/protocol/http/sampler/HttpSamplerManagerWarningsTest.java b/src/protocol/http/src/test/java/org/apache/jmeter/protocol/http/sampler/HttpSamplerManagerWarningsTest.java new file mode 100644 index 00000000000..4268b0eec99 --- /dev/null +++ b/src/protocol/http/src/test/java/org/apache/jmeter/protocol/http/sampler/HttpSamplerManagerWarningsTest.java @@ -0,0 +1,298 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.jmeter.protocol.http.sampler; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.stream.Stream; + +import org.apache.jmeter.config.KeystoreConfig; +import org.apache.jmeter.protocol.http.control.AuthManager; +import org.apache.jmeter.protocol.http.control.CacheManager; +import org.apache.jmeter.protocol.http.control.CookieManager; +import org.apache.jmeter.protocol.http.control.DNSCacheManager; +import org.apache.jmeter.protocol.http.control.HeaderManager; +import org.apache.jmeter.protocol.http.control.gui.HttpTestSampleGui; +import org.apache.jmeter.testelement.TestElement; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.core.LoggerContext; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +/** + * Regression tests for spurious "Existing XxxManager ... superseded by ..." warnings + * fired on every thread-group iteration after the first one. + * + *

Root cause: {@link HTTPSamplerBase#addTestElement(TestElement)} routed CookieManager, + * CacheManager, AuthManager and DNSCacheManager through their public {@code setXxxManager} + * methods, which log a warning whenever the existing manager is non-null. Since + * {@code clearTestElementChildren()} only clears the HeaderManager property, the manager + * reference survives across iterations and the warning fires N-1 times for N iterations.

+ * + *

Fix:

+ * + * + *

These tests verify both the {@code addTestElement} path (used by TestCompiler on each + * iteration) and the public setter path. Warnings are captured by redirecting the log4j2 + * "jmeter-log" file appender to a per-class temporary file via the {@code jmeter.logfile} + * system property (see {@code src/core/src/testFixtures/resources/log4j2.xml"}), so the + * tests do not depend on the working directory and can be re-run in isolation.

+ */ +public class HttpSamplerManagerWarningsTest { + + /** log4j2 system property used in src/core/src/testFixtures/resources/log4j2.xml. */ + private static final String JMeter_LOGFILE_PROPERTY = "jmeter.logfile"; + + private static Path logFile; + private static String originalLogfileProperty; + + private HTTPSamplerBase sampler; + + @BeforeAll + public static void redirectLogFile(@TempDir Path tempDir) { + logFile = tempDir.resolve("HttpSamplerManagerWarningsTest.log"); + originalLogfileProperty = System.getProperty(JMeter_LOGFILE_PROPERTY); + System.setProperty(JMeter_LOGFILE_PROPERTY, logFile.toAbsolutePath().toString()); + // Force log4j2 to re-read the configuration so the ${sys:jmeter.logfile:-jmeter.log} + // lookup in src/core/src/testFixtures/resources/log4j2.xml picks up the new path. + ((LoggerContext) LogManager.getContext(false)).reconfigure(); + } + + @AfterAll + public static void restoreLogFile() { + if (originalLogfileProperty == null) { + System.clearProperty(JMeter_LOGFILE_PROPERTY); + } else { + System.setProperty(JMeter_LOGFILE_PROPERTY, originalLogfileProperty); + } + ((LoggerContext) LogManager.getContext(false)).reconfigure(); + } + + @BeforeEach + public void setUp() { + sampler = (HTTPSamplerBase) new HttpTestSampleGui().createTestElement(); + } + + /** + * Provides the 5 replace-mode managers covered by the fix. + * Each argument is: (manager factory, display name). + */ + static Stream replaceModeManagers() { + return Stream.of( + Arguments.of((ManagerFactory) CookieManager::new, "CookieManager"), + Arguments.of((ManagerFactory) CacheManager::new, "CacheManager"), + Arguments.of((ManagerFactory) AuthManager::new, "AuthManager"), + Arguments.of((ManagerFactory) DNSCacheManager::new, "DNSCacheManager"), + Arguments.of((ManagerFactory) KeystoreConfig::new, "KeystoreConfig") + ); + } + + /** + * Scenario 1: simulate N thread-group iterations via {@code addTestElement}. + * Expected: zero "superseded by" warnings across all iterations. + */ + @ParameterizedTest(name = "{1}: addTestElement x5 should produce no warning") + @MethodSource("replaceModeManagers") + public void addTestElementAcrossIterationsShouldNotWarn( + ManagerFactory factory, String displayName) throws IOException { + T manager = factory.create(); + manager.setName(displayName + "-UnderTest"); + + long baseline = countSupersededWarningsInLog(); + for (int i = 1; i <= 5; i++) { + sampler.addTestElement(manager); + } + long delta = countSupersededWarningsInLog() - baseline; + assertEquals(0L, delta, + "[" + displayName + "] 5 addTestElement iterations produced " + delta + + " spurious 'superseded by' warnings (expected 0)"); + } + + /** + * Scenario 2: public setter called with the SAME instance should not warn (defensive guard). + * This protects against any caller that re-assigns the same manager instance. + */ + @ParameterizedTest(name = "{1}: setXxx(sameInstance) should not warn") + @MethodSource("replaceModeManagers") + public void setXxxManagerWithSameInstanceShouldNotWarn( + ManagerFactory factory, String displayName) throws IOException { + T manager = factory.create(); + manager.setName(displayName + "-Same"); + + // First call: sets the manager (no prior value, no warning expected) + long baseline = countSupersededWarningsInLog(); + setManagerOnSampler(displayName, manager); + long deltaAfterFirst = countSupersededWarningsInLog() - baseline; + assertEquals(0L, deltaAfterFirst, + "[" + displayName + "] initial setXxxManager should not warn"); + + // Second call with the same instance: must not warn + setManagerOnSampler(displayName, manager); + long deltaAfterSecond = countSupersededWarningsInLog() - baseline; + assertEquals(0L, deltaAfterSecond, + "[" + displayName + "] setXxxManager with same instance should not warn"); + } + + /** + * Scenario 3: public setter called with a DIFFERENT instance must still warn, so that + * genuine misuses (user attaching multiple managers of the same type) are still detected. + */ + @ParameterizedTest(name = "{1}: setXxx(differentInstance) should still warn") + @MethodSource("replaceModeManagers") + public void setXxxManagerWithDifferentInstanceShouldStillWarn( + ManagerFactory factory, String displayName) throws IOException { + T first = factory.create(); + first.setName(displayName + "-First"); + T second = factory.create(); + second.setName(displayName + "-Second"); + assertNotSame(first, second, "test fixture: two distinct instances expected"); + + setManagerOnSampler(displayName, first); + long baseline = countSupersededWarningsInLog(); + setManagerOnSampler(displayName, second); + long delta = countSupersededWarningsInLog() - baseline; + assertTrue(delta >= 1, + "[" + displayName + "] setXxxManager with a different instance must still warn" + + " (got " + delta + " warnings, expected >= 1)"); + } + + /** + * Scenario 4: HeaderManager uses merge (not replace) and is the original target of + * {@code clearTestElementChildren}. It must not produce "superseded by" warnings either, + * and the merge accumulation behaviour must keep working across iterations. + */ + @Test + public void headerManagerAddTestElementShouldNotWarnAndShouldMerge() throws IOException { + HeaderManager hm = new HeaderManager(); + hm.setName("HeaderManager-UnderTest"); + + long baseline = countSupersededWarningsInLog(); + for (int i = 1; i <= 5; i++) { + sampler.addTestElement(hm); + } + long delta = countSupersededWarningsInLog() - baseline; + assertEquals(0L, delta, + "HeaderManager 5 addTestElement iterations produced " + delta + + " 'superseded by' warnings (expected 0)"); + // HeaderManager is cleared each iteration and re-merged, so it must remain attached. + assertNotNull(sampler.getHeaderManager(), + "HeaderManager should remain attached after iterations"); + } + + /** + * Scenario 5: end-to-end smoke test that combines all 6 managers in a single sampler + * across 10 iterations. This mirrors the real-world pattern where a Thread Group has + * multiple config elements as children. + */ + @Test + public void allManagersTogetherAcrossIterationsShouldNotWarn() throws IOException { + CookieManager cookieManager = new CookieManager(); + cookieManager.setName("HTTP Cookie Manager"); + CacheManager cacheManager = new CacheManager(); + cacheManager.setName("HTTP Cache Manager"); + AuthManager authManager = new AuthManager(); + authManager.setName("HTTP Authorization Manager"); + DNSCacheManager dnsCacheManager = new DNSCacheManager(); + dnsCacheManager.setName("DNS Cache Manager"); + KeystoreConfig keystoreConfig = new KeystoreConfig(); + keystoreConfig.setName("Keystore Config"); + HeaderManager headerManager = new HeaderManager(); + headerManager.setName("HTTP Header Manager"); + + long baseline = countSupersededWarningsInLog(); + for (int i = 1; i <= 10; i++) { + sampler.addTestElement(cookieManager); + sampler.addTestElement(cacheManager); + sampler.addTestElement(authManager); + sampler.addTestElement(dnsCacheManager); + sampler.addTestElement(keystoreConfig); + sampler.addTestElement(headerManager); + } + long delta = countSupersededWarningsInLog() - baseline; + assertEquals(0L, delta, + "10 iterations with 6 managers produced " + delta + + " 'superseded by' warnings (expected 0)"); + + // Sanity: managers are still attached + assertNotNull(sampler.getCookieManager()); + assertNotNull(sampler.getCacheManager()); + assertNotNull(sampler.getAuthManager()); + assertNotNull(sampler.getDNSResolver()); + assertNotNull(sampler.getKeystoreConfig()); + assertNotNull(sampler.getHeaderManager()); + } + + // ---------------- helpers ---------------- + + /** + * Routes the given manager to the appropriate public setter on the sampler. + * Used by the parameterized tests so each manager type is exercised uniformly. + */ + private void setManagerOnSampler(String displayName, TestElement manager) { + switch (displayName) { + case "CookieManager" -> sampler.setCookieManager((CookieManager) manager); + case "CacheManager" -> sampler.setCacheManager((CacheManager) manager); + case "AuthManager" -> sampler.setAuthManager((AuthManager) manager); + case "DNSCacheManager" -> sampler.setDNSResolver((DNSCacheManager) manager); + case "KeystoreConfig" -> sampler.setKeystoreConfig((KeystoreConfig) manager); + default -> throw new IllegalArgumentException("Unknown manager type: " + displayName); + } + } + + private long countSupersededWarningsInLog() throws IOException { + return readSupersededLinesFromLog().size(); + } + + private List readSupersededLinesFromLog() throws IOException { + if (!Files.exists(logFile)) { + return new ArrayList<>(); + } + List hits = new ArrayList<>(); + for (String line : Files.readAllLines(logFile, StandardCharsets.UTF_8)) { + if (line.contains("superseded by")) { + hits.add(line); + } + } + return hits; + } + + @FunctionalInterface + interface ManagerFactory { + T create(); + } +} diff --git a/xdocs/changes.xml b/xdocs/changes.xml index 988377c81b1..d0ff1d7244e 100644 --- a/xdocs/changes.xml +++ b/xdocs/changes.xml @@ -116,6 +116,10 @@ Summary
  • 6620Fix report generation paths so dashboard output files are created in the correct location after internal refactoring.
  • 6456Handle malformed percent-encoded URLs gracefully when recording HTTP traffic, logging a warning instead of failing the recording.
    • +

    HTTP Samplers and Test Script Recorder

    +
      +
    • 6457Suppress spurious Existing CookieManager … superseded by … (and analogous CacheManager, AuthManager, DNSCacheManager, KeystoreConfig) warnings that fired on every thread-group iteration after the first one. addTestElement now routes replace-mode managers through their private property setters (mirroring the existing KeystoreConfig path), and the public setters no longer warn when the new value is the same instance as the existing one.
      • +
    Thanks