serhiy-bzhezytskyy opened a new pull request, #59:
URL: https://github.com/apache/solr-orbit/pull/59
### Description
`update_config_object` read `test_run.cluster_config_instance_params`, which
no test run has, so
`aggregate` always failed:
```
[ERROR] ❌ Cannot aggregate. 'TestRun' object has no attribute
'cluster_config_instance_params'.
```
`TestRun` defines `cluster_config_params` (`solrorbit/metrics.py:890`), and
the name being read occurs
nowhere else in the tree. Commit `2c85d505` ("Rebase renamed-components onto
latest developments")
renamed the attribute on `TestRun` to `cluster_config_params` but renamed
this read site to
`cluster_config_instance_params`; commit `e10342d1` ("fixes for aggregate
command") later corrected the
*config key* on that same line, from `cluster_config_instance.params` to
`cluster_config.params`, and
left the attribute name as it was. This reads the name `TestRun` actually
defines.
Unlike the null-metrics crash (#55), this one does not depend on the data —
it is on the plain
`aggregate` → `build_aggregated_results` → `update_config_object` path, so
it fires on every invocation.
As far as I can tell that means `aggregate` cannot currently succeed for
anyone on `main`.
A side effect of the fix: because the read never succeeded before, the
cluster-config params never reached
the aggregated result. They now do — my aggregates carry
`cluster-config-instance-params:
{"heap_size": "6g"}` as intended.
### Issues Resolved
Resolves #56
### Testing
- [x] New functionality includes testing
- One new test,
`test_update_config_object_reads_attributes_that_test_runs_actually_have`, which
constructs a **real** `metrics.TestRun` and asserts the
`cluster_config.params` value reaches
`config.add`.
- Verified red without the fix: with `main`'s `aggregator.py` restored under
the new test, it fails with
`AttributeError: 'TestRun' object has no attribute
'cluster_config_instance_params'`; with the fix it
passes.
- Full suite: `1100 passed, 5 skipped`. `ruff check` clean.
- End-to-end: `aggregate` completes on 3 configurations × 5 real runs of the
full geonames corpus.
The reason the existing suite doesn't catch this is worth flagging on its
own: every test in
`tests/aggregator_test.py` passes a bare `Mock()` as the test run, and a
`Mock` returns a fresh `Mock` for
any attribute name asked of it — so a wrong name reads fine and the suite
stays green over a command that
cannot run at all. `Mock(spec=TestRun)` does not fix it either, because
`spec` inspects the class, which
knows nothing about attributes assigned in `__init__`; it raises on
`cluster_config` first and points at
the wrong line. A real `TestRun` is the only thing that catches it, which is
why the new test builds one.
### Notes
Found while running a multi-configuration geonames campaign comparing Solr
on Lucene 10.4 against
Lucene 11. This was the second of three defects blocking `aggregate`; it
only became visible after #55
was fixed, because the null-metrics crash happens earlier on the same path.
The same code is present in OpenSearch Benchmark, which this is a port of,
so the defect looks inherited
rather than introduced here. I intend to report it upstream too.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
