vlsi commented on code in PR #6772:
URL: https://github.com/apache/jmeter/pull/6772#discussion_r4098259687
##########
xdocs/usermanual/component_reference.xml:
##########
@@ -6312,6 +6313,7 @@ generate the template string, and store the result into
the given variable name.
<li>Use a value of zero to indicate JMeter should choose a
match at random.</li>
<li>A positive number N means to select the n<sup>th</sup>
match.</li>
<li> Negative numbers are used in conjunction with the
<complink name="ForEach Controller"/> - see below.</li>
+ <li>If the field is left <b>empty</b>, it defaults to
<code>0</code> (random).</li>
Review Comment:
Same as the Regex Extractor section: `required="Yes"` contradicts this line,
and the wording can be simplified.
```suggestion
<li>An empty field is treated as <code>0</code>.</li>
```
##########
src/components/src/test/java/org/apache/jmeter/extractor/TestBoundaryExtractor.java:
##########
@@ -108,4 +110,65 @@ public void testOnlyRightBoundary() {
assertEquals("on", vars.get("varname_1"), "First match is incorrect");
assertEquals("1", vars.get("varname_matchNr"), "MatchNumber is
incorrect");
}
+
+ /**
+ * matchNumber=0 means random: when there is exactly one match the result
+ * must equal that match (no ambiguity about which one is chosen).
+ */
+ @Test
+ public void testMatchNumberZeroRandomSingleMatch() {
+ vars.put("content", "left-VALUE-right");
+ extractor.setLeftBoundary("left-");
+ extractor.setRightBoundary("-right");
+ extractor.setMatchNumber(0);
+ extractor.setRefName("varname");
+ extractor.setScopeVariable("content");
+ extractor.setThreadContext(jmctx);
+ extractor.process();
+ assertEquals("VALUE", vars.get("varname"),
+ "matchNumber=0 (random) with a single match should return that
match");
+ assertNull(vars.get("varname_matchNr"),
+ "matchNr variable should not be set for matchNumber=0");
+ }
+
+ /**
+ * matchNumber=0 means random: when there are multiple matches the result
+ * must be one of the available matches.
+ */
+ @Test
+ public void testMatchNumberZeroRandomMultipleMatches() {
+ vars.put("content", "left-A-right left-B-right left-C-right");
+ extractor.setLeftBoundary("left-");
+ extractor.setRightBoundary("-right");
+ extractor.setMatchNumber(0);
+ extractor.setRefName("varname");
+ extractor.setScopeVariable("content");
+ extractor.setThreadContext(jmctx);
+ extractor.process();
+ String found = vars.get("varname");
+ assertNotNull(found, "matchNumber=0 (random) should return a non-null
result when matches exist");
+ assertTrue("A".equals(found) || "B".equals(found) || "C".equals(found),
+ "matchNumber=0 (random) result '" + found + "' should be one
of the available matches");
+ assertNull(vars.get("varname_matchNr"),
+ "matchNr variable should not be set for matchNumber=0");
+ }
+
+ /**
+ * An empty Match No. field is stored as "" which resolves to 0 via
Review Comment:
This describes the implementation (`getIntValue()`), and it is not how
`RegexExtractor` reads the value (it goes through `RegexExtractorSchema`). The
comment will go stale on the next refactoring. State the rule instead: "An
empty Match No. is treated as 0."
##########
xdocs/usermanual/component_reference.xml:
##########
@@ -5744,6 +5744,7 @@ generate the template string, and store the result into
the given variable name.
<li>Use a value of zero to indicate JMeter should choose a
match at random.</li>
<li>A positive number N means to select the n<sup>th</sup>
match.</li>
<li> Negative numbers are used in conjunction with the
<complink name="ForEach Controller"/> - see below.</li>
+ <li>If the field is left <b>empty</b>, it defaults to
<code>0</code> (random).</li>
Review Comment:
The property above is declared `required="Yes"`, and this line documents
what happens when it is left empty. Please resolve the contradiction: either
change the attribute to `required="No"` (the XPath, JSON, and JMESPath sections
already do that), or drop this line.
The `<b>` emphasis is not used by the neighboring items, and "(random)"
repeats the first bullet. A plainer form:
```suggestion
<li>An empty field is treated as <code>0</code>.</li>
```
##########
src/components/src/test/java/org/apache/jmeter/extractor/TestBoundaryExtractor.java:
##########
@@ -108,4 +110,65 @@ public void testOnlyRightBoundary() {
assertEquals("on", vars.get("varname_1"), "First match is incorrect");
assertEquals("1", vars.get("varname_matchNr"), "MatchNumber is
incorrect");
}
+
+ /**
+ * matchNumber=0 means random: when there is exactly one match the result
+ * must equal that match (no ambiguity about which one is chosen).
+ */
+ @Test
+ public void testMatchNumberZeroRandomSingleMatch() {
+ vars.put("content", "left-VALUE-right");
+ extractor.setLeftBoundary("left-");
+ extractor.setRightBoundary("-right");
+ extractor.setMatchNumber(0);
+ extractor.setRefName("varname");
+ extractor.setScopeVariable("content");
+ extractor.setThreadContext(jmctx);
+ extractor.process();
+ assertEquals("VALUE", vars.get("varname"),
+ "matchNumber=0 (random) with a single match should return that
match");
+ assertNull(vars.get("varname_matchNr"),
+ "matchNr variable should not be set for matchNumber=0");
+ }
+
+ /**
+ * matchNumber=0 means random: when there are multiple matches the result
+ * must be one of the available matches.
+ */
+ @Test
+ public void testMatchNumberZeroRandomMultipleMatches() {
+ vars.put("content", "left-A-right left-B-right left-C-right");
+ extractor.setLeftBoundary("left-");
+ extractor.setRightBoundary("-right");
+ extractor.setMatchNumber(0);
+ extractor.setRefName("varname");
+ extractor.setScopeVariable("content");
+ extractor.setThreadContext(jmctx);
+ extractor.process();
+ String found = vars.get("varname");
+ assertNotNull(found, "matchNumber=0 (random) should return a non-null
result when matches exist");
+ assertTrue("A".equals(found) || "B".equals(found) || "C".equals(found),
Review Comment:
The test is named `...Random...`, but it still passes when the extractor
always returns the first match, so it does not check randomness. Please rename
it to what it asserts (the value is one of the matches), or drop "Random" from
the name.
`assertNotNull` above is redundant: a `null` value already fails this check.
And a boolean `assertTrue` over `a || b || c` does not print the operands; the
message covers that here, but `assertTrue(Set.of("A", "B",
"C").contains(found), ...)` states the intent directly.
##########
src/components/src/test/java/org/apache/jmeter/extractor/TestRegexExtractor.java:
##########
@@ -125,6 +125,21 @@ public void testVariableExtraction0() {
assertEquals("value", vars.get("regVal"));
}
+ /**
+ * An empty Match No. field must behave identically to matchNumber=0
(random).
+ * When there is exactly one match the result must equal that match.
Review Comment:
The fixture in `setUp` has nine `<value field="` elements, so `<(value)
field="` matches nine times, not once, and every match renders as `value`. The
comment is wrong, and the test cannot tell "first match", "random match", and
"N-th match" apart.
It is also `testVariableExtraction0` a few lines above with `""` instead of
`0`. Please replace it with a check that can fail, for example `assertEquals(0,
extractor.getMatchNumber())` after `setMatchNumber("")`, and fix the comment.
##########
src/components/src/test/java/org/apache/jmeter/extractor/TestBoundaryExtractor.java:
##########
@@ -108,4 +110,65 @@ public void testOnlyRightBoundary() {
assertEquals("on", vars.get("varname_1"), "First match is incorrect");
assertEquals("1", vars.get("varname_matchNr"), "MatchNumber is
incorrect");
}
+
+ /**
+ * matchNumber=0 means random: when there is exactly one match the result
+ * must equal that match (no ambiguity about which one is chosen).
+ */
+ @Test
+ public void testMatchNumberZeroRandomSingleMatch() {
+ vars.put("content", "left-VALUE-right");
+ extractor.setLeftBoundary("left-");
+ extractor.setRightBoundary("-right");
+ extractor.setMatchNumber(0);
+ extractor.setRefName("varname");
+ extractor.setScopeVariable("content");
+ extractor.setThreadContext(jmctx);
+ extractor.process();
+ assertEquals("VALUE", vars.get("varname"),
+ "matchNumber=0 (random) with a single match should return that
match");
+ assertNull(vars.get("varname_matchNr"),
+ "matchNr variable should not be set for matchNumber=0");
+ }
+
+ /**
+ * matchNumber=0 means random: when there are multiple matches the result
+ * must be one of the available matches.
+ */
+ @Test
+ public void testMatchNumberZeroRandomMultipleMatches() {
+ vars.put("content", "left-A-right left-B-right left-C-right");
+ extractor.setLeftBoundary("left-");
+ extractor.setRightBoundary("-right");
+ extractor.setMatchNumber(0);
+ extractor.setRefName("varname");
+ extractor.setScopeVariable("content");
+ extractor.setThreadContext(jmctx);
+ extractor.process();
+ String found = vars.get("varname");
+ assertNotNull(found, "matchNumber=0 (random) should return a non-null
result when matches exist");
+ assertTrue("A".equals(found) || "B".equals(found) || "C".equals(found),
+ "matchNumber=0 (random) result '" + found + "' should be one
of the available matches");
+ assertNull(vars.get("varname_matchNr"),
+ "matchNr variable should not be set for matchNumber=0");
+ }
+
+ /**
+ * An empty Match No. field is stored as "" which resolves to 0 via
+ * getIntValue(), so it must behave identically to matchNumber=0 (random).
+ */
+ @Test
+ public void testEmptyMatchNumberFieldBehavesLikeZero() {
Review Comment:
With a single match, this test passes no matter what an empty field means:
`0`, `1`, or any positive number. So it cannot fail on the alternative #6378
asks about (empty means the first match), and the name "BehavesLikeZero" is not
what it checks.
A deterministic check of the documented rule:
```java
extractor.setMatchNumber("");
assertEquals(0, extractor.getMatchNumber(), "getMatchNumber() for an empty
Match No.");
```
To keep a behavioral test, feed several matches and assert what separates
`0` from `-1`: `varname` is set, and `varname_1` and `varname_matchNr` are not.
##########
src/components/src/test/java/org/apache/jmeter/extractor/TestBoundaryExtractor.java:
##########
@@ -108,4 +110,65 @@ public void testOnlyRightBoundary() {
assertEquals("on", vars.get("varname_1"), "First match is incorrect");
assertEquals("1", vars.get("varname_matchNr"), "MatchNumber is
incorrect");
}
+
+ /**
+ * matchNumber=0 means random: when there is exactly one match the result
+ * must equal that match (no ambiguity about which one is chosen).
+ */
+ @Test
+ public void testMatchNumberZeroRandomSingleMatch() {
Review Comment:
`BoundaryExtractorTest.kt` already covers this case: `ExtractCase(1..1, 0,
listOf("1"))` in `extractCases()`. The only new check here is `varname_matchNr`
being unset. Please either drop this test, or add that one assertion to the
existing Kotlin tests so the case lives in one place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
