Bugfix: User Defined Variables are not evaluated on client (master) i…

[markuswege]

Description

Bugfix: #5896

User Defined Variables are not evaluated on client (master) in distributed setups. This has beed fixed by a second pass through the test tree after it is sent to the servers.

Motivation and Context

We are using a distributed setup for our tests. Credentials and some configurations are passed via properties into the client (master). It occurred that the BackendListener, using User Defined Variables, received unevaluated strings as credentials (see bugticket linked for details).
In order to prevent side effects for test party running on the servers, we decided a second pass over the needed testelements after sending the unmodified test to the servers would be best.

How Has This Been Tested?

Manual testing using debug logging.

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code follows the code style of this project.
• I have updated the documentation accordingly.

[vlsi]

Please keep .idea/icon.png intact.

[vlsi]

Do you think you could add a test case for the change?

[markuswege]

Stripped.zip

Testfile attached. To be run in distributed mode with

-JactiveProfileId=StrippedProfile -GactiveProfileId=StrippedProfile -Jpassword_influxdb_qat=PWD

[vlsi]

Can you wrap it as a batch test server test?

See https://github.com/apache/jmeter/blob/193bd552d492628e31b4c72bc2a5085ed0b14126/src/dist-check/build.gradle.kts#LL233C1-L233C1 , https://github.com/apache/jmeter/blob/193bd552d492628e31b4c72bc2a5085ed0b14126/bin/testfiles/BatchTestLocal.jmx

[markuswege]

After some checking i don't think placing this in the batch tests makes sense, since we would need an assertion inside of the backend listener- The attached file was used for our manual tests to check for the correct behavior.
Also i checked the unittests. As far as i see there are currently no tests for the ClientJMeterEngine, which calls the necessary parts of the PreCompiler class change in this fix.

[vlsi]

Please include the tests in the PR. For now, I see only ClientJMeterEngine and PreCompiler changes.

The PRs that alter core behaviours must be accompanied by tests.

[markuswege]

Undestandable.
So, why are there no tests for ClientJmeterEngine?
Test coverage run in IntelliJ over all included tests show no coverage of ClientJMeter Engine or the according if-branch in PreCompiler.
If there were Tests, i would have added Testscases, but i simply don't have the time to create Tests for ClientJMeterEngine and PreCompiler from scratch.

[vlsi]

So, why are there no tests for ClientJmeterEngine?

I have no idea.
It can be some tests are missing.
It can be the tests are executed from certain batch tests only.
It can be IntelliJ code coverage does not track tests spawned from batch tests because they execute JMeter as a custom subprocess, so they don't automatically inherit IntelliJ code coverage agent options.

---

but i simply don't have the time to create Tests for ClientJMeterEngine and PreCompiler from scratch.

I understand what you say.
However, at the same time, I would mention that the code without tests is hard to maintain.

Imagine some time later I would start working on #5877
What would I do with the code you suggest in this PR? It would be hard to "just drop the code", however, it would be hard to understand if it still works as intended. That is one of the "maintenance overheads" we have.

The bug #5896 does not affect me directly, and I have no time for looking into it either.
So it would be great if you could reconsider and spend a little bit of time creating tests to cover your changes.
An alternative option might be waiting for somebody else to do it.
Thank you for understanding.

[vlsi]

So, why are there no tests for ClientJmeterEngine?

Try adding if (true) { throw new IllegalStateException("ClientJmeterEngine is broken"); } to ClientJmeterEngine in your PR.
Then you'll see what are the tests that can detect such a change.

[markuswege]

After i while i refuse to write a test for this.
Why?
Because there is simply not a single test for the core classes. Few parts are covered indeirectly by tests.

Nevertheless, as you say: Hard to maintain, also hard to write tests for a pretty simple fix without any other tests doing some basic stuff like creating the TestTree from scratch.

Please either check manually or trust in our team programming efforts to fix this.

[vlsi]

hard to write tests for a pretty simple fix without any other tests doing some basic stuff like creating the TestTree from scratch.

Please check

jmeter/src/components/src/test/kotlin/org/apache/jmeter/threads/openmodel/OpenModelThreadGroupConfigElementTest.kt

Lines 39 to 85 in 5bb5669

	fun `ensure thread group initializes counter only once for each thread`() {
	val listener = TestTransactionController.TestSampleListener()
	val tree = ListedHashTree().apply {
	add(TestPlan()).apply {
	val threadGroup = OpenModelThreadGroup().apply {
	name = "Thread Group"
	// 5 samples within 100ms
	// Then 2 sec pause to let all the threads to finish, especially the ones that start at 99ms
	scheduleString = "rate(50 / sec) random_arrivals(100 ms) pause(2 s)"
	}
	add(threadGroup).apply {
	add(listener)
	add(
	CounterConfig().apply {
	varName = "counter"
	increment = 1
	}
	)
	add(
	DebugSampler().apply {
	name = "\${counter}"
	isDisplayJMeterProperties = false
	isDisplayJMeterVariables = false
	isDisplaySystemProperties = false
	}
	)
	}
	}
	}
	StandardJMeterEngine().apply {
	configure(tree)
	runTest()
	awaitTermination(Duration.ofSeconds(10))
	}
	// There's no guarantee that threads execute exactly in order, so we sort
	// the labels to avoid test failure in case the thread execute out of order.
	val actual = listener.events.map { it.result.sampleLabel }.sorted()
	// Use toString for better error message
	Assert.assertEquals(
	"Counter values should be consequent",
	"0\n1\n2\n3\n4",
	actual.joinToString("\n")
	)

It creates the test tree from scratch.

[markuswege]

Note: We found a Workaround which was easier to implement as the tests for this fix ;)