serhiy-bzhezytskyy opened a new pull request, #60:
URL: https://github.com/apache/solr-orbit/pull/60
### Description
`add_workload_source()` offers a workload either as a repository or as a
local path, and `run`, `list` and
`info` all use it. `aggregate` defined only its own `--workload-repository`,
so a workload developed
locally and benchmarked with `run --workload-path` could not be aggregated
afterwards:
```
solr-orbit: error: unrecognized arguments:
--workload-path=/path/to/my-workload
```
`aggregate` now uses the same helper, so it accepts either source, keeps the
two mutually exclusive, and
gains `--workload-revision` for free. It also goes through
`configure_workload_params`, so a path is
normalised and validated the way it is everywhere else — that is also what
rejects `--workload-revision`
together with `--workload-path`. `command_requires_workload=False` because
`aggregate` takes the workload
name from the stored test run, not from the command line.
One behavioural detail: a path sets `workload.path`, which is what makes the
loader choose
`SimpleWorkloadRepository`. So when a path was given, `aggregate()` no
longer names a repository — doing
both would send the loader looking for the workload inside that repository
instead. That guard is
load-bearing, since argparse's default for `--workload-repository` is the
string `"default"`, not `None`.
### Issues Resolved
Resolves #57
### Testing
- [x] New functionality includes testing
- Two new tests: `test_aggregate_names_the_workload_repository` (unchanged
behaviour when a repository is
used) and `test_aggregate_leaves_a_workload_path_alone` (no
`repository.name` is configured when a path
is given).
- The shared `mock_args` fixture previously supplied no `workload_path`, so
the aggregator saw a `Mock`
attribute that happens to be truthy. It is now explicit — that fixture
change is what makes the first
test meaningful rather than accidentally passing.
- CLI verified by hand on this branch: `aggregate --workload-path=...` is
accepted where it previously
errored, and combining it with `--workload-revision` is correctly rejected.
- Full suite: `1100 passed, 5 skipped`, plus one unrelated pre-existing
flake described below.
`ruff check` clean.
On the flake, so it isn't mistaken for fallout from this PR:
`tests/workload/workload_test.py::DocumentCorpusTests::test_union_document_corpora_is_symmetric`
fails on
roughly one interpreter start in eight. `DocumentCorpus.union` builds its
document list with
`list(set(...).union(...))` while `DocumentCorpus.__eq__` compares that list
in order, so symmetry depends
on hash iteration order. It reproduces deterministically under
`PYTHONHASHSEED=7` on `main`'s untouched
workload code and on the other two fix branches, and this branch does not
touch `solrorbit/workload/` at
all. I'll report it separately rather than fold an unrelated fix in here.
### Notes
Found while running a multi-configuration geonames campaign comparing Solr
on Lucene 10.4 against
Lucene 11. Third of three `aggregate` gaps, and the only one that is a
parity gap rather than a bug.
There is a case for going further — a test run already records the workload
it used, so `aggregate` could
resolve the spec from the stored run and need no workload-source argument at
all. I've gone with the
parity fix because it is the smaller change and matches the existing
pattern, but I'm open to the other
shape if you'd rather have it.
--
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]
