refactor: remove integration tests and sim_adapter#140
Conversation
hlin99-Review-Bot
left a comment
There was a problem hiding this comment.
CI is failing because .github/workflows/ci.yml still references the deleted directories:
- test job:
python -m pytest tests/unit/ tests/integration/ -v—tests/integration/no longer exists. - build job:
pip install ... xpyd-sim && python -m pytest tests/integration/test_cli_and_discovery.py -v— file no longer exists, andxpyd-simis no longer a dependency. - benchmark job:
python -m pytest tests/stress/ -v—tests/stress/no longer exists.
The PR removes the test files and xpyd-sim dep but does not update the CI workflow. Please update .github/workflows/ci.yml to:
- Remove
tests/integration/from the test command (just runtests/unit/). - Remove or update the build job that runs
test_cli_and_discovery.py. - Remove or update the benchmark job that runs
tests/stress/.
Also: no linked GitHub Issue found in the PR description. If this relates to an issue, please reference it.
hlin99-Review-BotX
left a comment
There was a problem hiding this comment.
CI is red — the workflow (.github/workflows/ci.yml) still references the deleted directories and the removed xpyd-sim dependency:
- test job: runs
tests/integration/which no longer exists. - build job: installs
xpyd-simand runstests/integration/test_cli_and_discovery.py— both removed. - benchmark job: runs
tests/stress/which no longer exists.
The CI workflow must be updated to match the removed files. Agree with hlin99-Review-Bot's earlier review — this is still unresolved on the current commit.
Additionally, no linked GitHub Issue in the PR description.
|
Fixed in f99078d. |
hlin99-Review-Bot
left a comment
There was a problem hiding this comment.
The CI workflow updates address the previous review feedback (removed integration/stress/benchmark references), but the build job is now failing with 20 test failures.
Root cause: The build job installs only pytest (pip install pytest) then runs the full unit test suite (python -m pytest tests/unit/ -v). However, several unit tests in test_discovery_models.py and test_pd_metrics.py are async (@pytest.mark.anyio) and require pytest-asyncio (or anyio) to run. Since the build job only installs bare pytest, all async tests fail with async def functions are not natively supported.
Fix options:
- Change the build job test step to also install
pytest-asyncio:pip install pytest pytest-asyncio && python -m pytest tests/unit/ -v --tb=short - Or skip the unit test run in the build job entirely (its purpose is to verify the wheel installs and
xpyd --helpworks — thetestjob already runs the full suite with.[dev]).
Option 2 seems cleaner since the test matrix already covers unit tests thoroughly.
The rest of the PR looks good — file deletions are correct, .isort.cfg updated, pyproject.toml cleaned up, test_metrics.py integration tests properly removed, README updated. No issues with the code changes themselves.
| benchmark: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
This step installs only pytest but runs unit tests that include async tests requiring pytest-asyncio. Either install pytest-asyncio too, or remove this test step from the build job (the test matrix already covers unit tests with full dev deps).
hlin99-Review-BotX
left a comment
There was a problem hiding this comment.
Previous CI issues (deleted directories referenced in workflow) are fixed, but the build job is still failing.
Problem: The build job step pip install pytest && python -m pytest tests/unit/ -v --tb=short installs only bare pytest, but several unit tests (test_discovery_models.py, test_pd_metrics.py) use @pytest.mark.anyio and require pytest-asyncio. This causes 20 test failures with async def functions are not natively supported.
Suggested fix: Either:
- Remove the unit test step from the build job entirely — the
testmatrix already runs the full suite with.[dev]deps. The build job purpose is to verify the wheel installs andxpyd --helpworks. - Or install
pytest-asyncioas well:pip install pytest pytest-asyncio.
Option 1 is cleaner since it avoids duplicating test coverage.
Also: no linked GitHub Issue in the PR description (noted in previous review as well).
f99078d to
01dce96
Compare
|
Fixed in 93d1033 — removed the redundant unit test step from the build job as suggested. The test matrix already covers unit tests with full dev deps. Build job now only verifies wheel install + |
Tests migrated to xPyD-integration. Remove integration/ + stress/ + sim_adapter.py + conftest.py + xpyd-sim dep.
Removed:
tests/integration/(~115 tests)tests/stress/(~10 tests)sim_adapter.pytests/conftest.py(only used by integration tests)test_metrics.py(usedclientfixture from conftest)xpyd-simdev dependency from pyproject.tomlRemaining: 393 unit tests, all passing.