apache / apache/trafficserver

tests: core-task plugin-tag assertion in config_reload_plugin_api cannot fail

Open
#13,617 0 comments 0 reactions 1 assignee Claimed by @brbzull0 View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.