Fix aggregate reading a TestRun attribute that does not exist - #1097
Merged
OVI3D0 merged 1 commit intoJul 28, 2026
Merged
Conversation
`aggregate` fails immediately for every invocation:
[ERROR] Cannot aggregate. 'TestRun' object has no attribute
'cluster_config_instance_params'.
update_config_object reads `test_run.cluster_config_instance_params`, but
TestRun defines `cluster_config_params`. The name being read occurs nowhere
else in the repository, so the aggregated results never carried the cluster
config params they were meant to.
The existing tests pass bare Mock() test runs, and a Mock returns a new Mock
for any attribute name asked of it, so they could not catch this. The
regression test constructs a real TestRun instead. Note that Mock(spec=TestRun)
would not help either: spec inspects the class and knows nothing about
attributes assigned in __init__.
Signed-off-by: Serhiy Bzhezytskyy <me@serhiy-bzhezytskyy.com>
serhiy-bzhezytskyy
requested review from
IanHoang,
OVI3D0,
VijayanB,
beaioun,
gkamat and
rishabh6788
as code owners
July 25, 2026 15:11
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
OVI3D0
approved these changes
Jul 28, 2026
OVI3D0
left a comment
Member
There was a problem hiding this comment.
LGTM, thanks @serhiy-bzhezytskyy !
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
update_config_objectreadtest_run.cluster_config_instance_params, which no test run has, soaggregatefailed on every invocation:TestRundefinescluster_config_params(osbenchmark/metrics.py:1343), and the name being read occurs nowhere else in the repository — nothing assigns it. #882 (2c85d505) renamed the attribute onTestRuntocluster_config_paramsbut renamed this read site tocluster_config_instance_params; #939 (e10342d1) later corrected the config key on that same line, fromcluster_config_instance.paramstocluster_config.params, and left the attribute name as it was. This one-word change reads the nameTestRunactually defines.Unlike the null-metrics crash (#1093), this does not depend on the data — it is on the plain
aggregate→build_aggregated_results→update_config_objectpath. As far as I can tell it meansaggregatecannot currently succeed for anyone onmain.A side effect of the fix: because the read never succeeded, the cluster-config params never reached the aggregated result. They now do.
Issues Resolved
Resolves #1094
Testing
New functionality includes testing
One new test,
test_update_config_object_reads_attributes_that_test_runs_actually_have, which constructs a realmetrics.TestRunand asserts thecluster_config.paramsvalue reachesconfig.add.Verified red without the fix: with
main'saggregator.pyrestored under the new test it fails withAttributeError: 'TestRun' object has no attribute 'cluster_config_instance_params'; with the fix it passes.Full suite:
1423 passed, 5 skipped(baseline onmainis 1422).pylintclean.End-to-end through the real CLI:
aggregatereports✅ SUCCESSand the aggregated result now carriescluster-config-instance-params: {'heap_size': '6g'}, which was being silently lost.The reason the existing suite doesn't catch this is worth flagging on its own:
tests/aggregator_test.pynever constructs a realTestRun— every test passes a bareMock(), and aMockreturns a freshMockfor any attribute name asked of it, so a wrong name reads fine.mainsits at 1422 passed whileaggregatecannot complete once.Mock(spec=TestRun)does not fix it either, becausespecinspects the class, which knows nothing about attributes assigned in__init__; it raises oncluster_configfirst and points at the wrong line. A realTestRunis the only thing that catches it, which is why the new test builds one.Notes
Found while using Apache solr-orbit, a Python port of OSB whose
aggregator.pyis byte-identical to this one apart from the import module name, to run a multi-configuration benchmark campaign. The same fix is proposed there as apache/solr-orbit#59.This was the second of three defects blocking
aggregate; it only became visible after the null-metrics crash was fixed, because that one happens earlier on the same path.