tests: core-task plugin-tag assertion in config_reload_plugin_api cannot fail
- Dominant language
- C++
- Stars
- 2k
- Forks
- 874
- Avg merge
- 6d 15h
- Merged PRs (30d)
- 46
Description
### Impact
Test-only defect, no production impact. Introduced by #13146, now on `master`.
Test G of `config_reload_plugin_api.test.py` is the only test asserting that a **core**
reload task does not carry the `[plugin: ]` attribution tag that
`traffic_ctl config status` adds for plugin-owned tasks. `ExcludesExpression` compiles its
first argument as a regex, and the unescaped brackets in `'ip_allow [plugin]'` make it a
character class rather than a literal. The pattern cannot match the command's output in
the correct case *or* in the regressed case, so the assertion passes unconditionally.
If core reload tasks ever started being reported as plugin-owned, this test would still go
green and the regression would ship undetected. Nothing else covers that behaviour.
```
Version: master @ 83335d4eff9694061409d889c73627a847f46d24
Platform: test-only; not platform specific
Config: none
```
### Proof
`ExcludesExpression` compiles its first argument as a regex and applies `re.search` per
line, and `IncludesExpression` is an alias of `ContainsExpression`, so all three matchers
behave this way:
```python
# autest/testers/__init__.py:16
from .contains_expression import ContainsExpression as IncludesExpression
# autest/testers/excludes_expression.py:38-42
if isinstance(regexp, str):
regexp = re.compile(regexp, reflags)
```
The assertion, at
https://github.com/apache/trafficserver/blob/83335d4eff9694061409d889c73627a847f46d24/tests/gold_tests/jsonrpc/config_reload_plugin_api.test.py#L271-L272
```python
tr.Processes.Default.Streams.stdout = Testers.ExcludesExpression(
'ip_allow [plugin]', 'Core task ip_allow must not have [plugin] tag')
```
`[plugin]` is a character class matching one of `p l u g i n`, so the pattern is
`ip_allow ` followed by one of those six characters. The tag it is meant to detect is
built at
https://github.com/apache/trafficserver/blob/83335d4eff9694061409d889c73627a847f46d24/src/traffic_ctl/CtrlPrinters.cc#L345-L350
```cpp
// Build label and right-aligned duration
std::string label = std::string(status_icon(f.status)) + " " + fname;
if (!f.meta.plugin_name.empty()) {
label += " [plugin: " + f.meta.plugin_name + "]";
}
```
so the character following `ip_allow ` is `[`, which is not in the class. Found by
inspection, then confirmed by running the exact pattern the test passes:
```python
import re
pat = re.compile('ip_allow [plugin]') # exactly what the test passes
good = " * ip_allow ................ 12ms" # core task, no tag
bad = " * ip_allow [plugin: my_plugin] .... 12ms" # the regression it guards
for line in (good, bad):
print(bool(pat.search(line)))
```
Observed:
```
core task (want: no match) search=False ExcludesExpression -> PASS
wrongly tagged (want: MATCH) search=False ExcludesExpression -> PASS
```
Expected, the second case must fail. With `r'ip_allow.*\[plugin:'`:
```
core task (want: no match) search=False ExcludesExpression -> PASS
wrongly tagged (want: MATCH) search=True ExcludesExpression -> FAIL
```
The two positive assertions in the same file are already written correctly as
`r'\[plugin: '` (lines 130 and 167); this negative one was missed.
A sweep of all 502 files under `tests/gold_tests` on `master` found 12 other bracketed
matchers, all of them the intentional `[Uu]sing HTTP/?2` in the `h2` tests. This is the
only instance of the defect.
### Proposed change
```diff
--- a/tests/gold_tests/jsonrpc/config_reload_plugin_api.test.py
+++ b/tests/gold_tests/jsonrpc/config_reload_plugin_api.test.py
@@ -268,8 +268,8 @@
tr.Processes.Default.Env = ts.Env
tr.Processes.Default.ReturnCode = 0
-tr.Processes.Default.Streams.stdout = Testers.ExcludesExpression(
- 'ip_allow [plugin]', 'Core task ip_allow must not have [plugin] tag')
+tr.Processes.Default.Streams.stdout = Testers.ExcludesExpression(
+ r'ip_allow.*\[plugin:', 'Core task ip_allow must not have a [plugin: ] tag')
tr.StillRunningAfter = ts
```
`.*` rather than a literal space, because the label goes through `sanitize_label()` and a
dot fill sits between the task name and the duration, so the tag is not adjacent to the
name at every column width. Matching the `[plugin:` prefix rather than a full
`[plugin: ]` keeps the assertion independent of the plugin name.
No production code changes, no compatibility impact, nothing to backport beyond wherever
#13146 lands.
Contributor guide
Assessment
This issue has not been assessed yet.