Skip to content

Use esrally/race check= consistently in integration tests#2099

Open
fressi-elastic wants to merge 6 commits into
elastic:masterfrom
fressi-elastic:fix-it-esrally
Open

Use esrally/race check= consistently in integration tests#2099
fressi-elastic wants to merge 6 commits into
elastic:masterfrom
fressi-elastic:fix-it-esrally

Conversation

@fressi-elastic

Copy link
Copy Markdown
Contributor

This updates integration tests to rely on esrally / race with check=True by default instead of assert ... == 0, and uses check=False with CompletedProcess.returncode where the test needs a non-zero exit or an explicit success check.

Also fixes the pytest.fail message in it.esrally (f-string), handles TestCluster.stop via returncode, imports CompletedProcess, and expands docstrings for esrally and race without Sphinx roles in Python strings.

Made with Cursor

- Default check=True: replace assert ... == 0 with bare calls that fail via pytest.fail on error
- Use check=False with CompletedProcess.returncode where tests assert non-zero or need explicit checks
- Fix pytest.fail f-string, TestCluster.stop returncode handling, import CompletedProcess
- Expand esrally and race docstrings (no Sphinx roles in Python)

Made-with: Cursor
Allow passing a custom subprocess environment from integration tests.
Proxy ITs call esrally with proxy vars instead of run_subprocess_with_output.

Made-with: Cursor
Document parameters, check/env/kwargs behavior, and when to use esrally
instead of race for custom subprocess options.

Made-with: Cursor

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request refactors the integration test helpers and test suite to run esrally / esrally race via wrappers that default to check=True, returning subprocess.CompletedProcess for cases where tests need to inspect output or assert non-zero exits.

Changes:

  • Update it.esrally() / it.race() to return CompletedProcess, default to check=True, and improve failure messaging/docstrings.
  • Replace many assert ... == 0 patterns in integration tests with wrapper calls that fail via check=True by default.
  • Update selected tests that need non-zero exit assertions to use check=False and check returncode.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
it/init.py Reworks esrally()/race() wrappers to return CompletedProcess, default check=True, and centralize subprocess execution.
it/tracker_test.py Adopts new race() / esrally() invocation style for track creation and benchmarks.
it/static_responses_test.py Switches race invocation to rely on wrapper check=True behavior.
it/sources_test.py Switches race/build invocations to rely on wrapper check=True behavior.
it/proxy_test.py Migrates subprocess execution to it.esrally(..., env=...) and asserts on captured stdout.
it/list_test.py Removes explicit return-code assertions in favor of wrapper check=True.
it/info_test.py Removes explicit return-code assertions in favor of wrapper check=True for success cases.
it/esrallyd_test.py Uses check=False + returncode assertion for a race invocation prior to log assertions.
it/download_test.py Uses wrapper for success cases and check=False + returncode for unsupported version case.
it/distribution_test.py Uses wrapper for success cases and check=False + returncode where failure is expected.
it/dependencies_test.py Switches race invocation to rely on wrapper check=True behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread it/__init__.py
Comment thread it/__init__.py Outdated
Comment thread it/__init__.py Outdated
Comment thread it/tracker_test.py Outdated
Comment thread it/esrallyd_test.py Outdated
- basic_test: assert on esrally stdout/stderr; tmpdir fixture for RALLY_HOME
- error_test: drop process helper; use esrally with check=False
- distribution_test: interrupt command uses --configuration-name; annotate interrupt helper return
- esrally: use err.cmd in pytest.fail; drop shlex import

Made-with: Cursor

@fressi-elastic fressi-elastic left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolving Copilot review threads (explanation)

  1. esrally_command_line_for — The helper was removed; the shell command is built only inside `it.esrally`. All former call sites use `it.esrally` / `it.race` or an explicit full command where subprocess needs a raw string (e.g. interrupt test). There are no remaining references to `esrally_command_line_for` on this branch.

  2. shlex.join(err.args) — The failure path now uses `err.cmd` in the `pytest.fail` message instead of `shlex.join(err.args)`.

  3. json.loads / `"".join(result.stdout)` — `TestCluster.install` now uses `json.loads(result.stdout)`.

  4. tracker_test `check=False` — Simplified to `it.race(cfg, cmd)` with default `check=True` for consistent failure reporting.

  5. esrallyd_test — Same: plain `it.race(...)` with default `check=True`, no redundant manual `returncode` check.

- info_test: use it.esrally(check=False) for invalid track-params; drop process helper
- tracker_test / esrallyd_test: rely on race() default check=True
- TestCluster.install: json.loads(result.stdout) instead of join on string

Made-with: Cursor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants