Move ESI runtime tests into pytest suite#9655
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request migrates ESI runtime and PyCDE integration tests from a lit-based testing framework to pytest, consolidating them under the ESI runtime package. The changes enable running tests through pytest with proper dependency management and improved test organization.
Changes:
- Migrated unit tests from
integration_test/Dialect/ESI/runtime/type_construction.pytolib/Dialect/ESI/runtime/tests/unit/test_types.pywith updated type API usage (usingis_valid()andfieldsinstead ofsupports_hostandfield()) - Created new pytest-based integration test suite in
lib/Dialect/ESI/runtime/tests/integration/test_cosim.pywith runtime root auto-detection, Verilator simulation support, and parametrized test cases - Added pytest configuration with test exclusions for hw/sw script directories, and pytest extras dependencies in pyproject.toml
Reviewed changes
Copilot reviewed 5 out of 21 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/Dialect/ESI/runtime/tests/unit/test_types.py | New unit test for ESI type construction with updated API (is_valid, fields) |
| lib/Dialect/ESI/runtime/tests/integration/test_cosim.py | Pytest-based cosim integration tests with runtime root detection and parametrized test cases |
| lib/Dialect/ESI/runtime/tests/integration/sw/*.py | Software test scripts (loopback, esi_test, esi_ram, esi_advanced, xrt_cosim, kanagawa_demo) |
| lib/Dialect/ESI/runtime/tests/integration/sw/loopback.cpp | C++ test for loopback with generated ESI headers |
| lib/Dialect/ESI/runtime/tests/integration/sw/CMakeLists.txt | CMake build configuration for C++ loopback test with runtime library detection |
| lib/Dialect/ESI/runtime/tests/integration/sw/lit.local.cfg.py | Lit config to exclude sw directory from lit test collection |
| lib/Dialect/ESI/runtime/tests/integration/hw/*.py | Hardware generation scripts (loopback, esitester, esi_test, esi_ram, esi_advanced) |
| lib/Dialect/ESI/runtime/tests/integration/init.py | Package marker for integration tests |
| lib/Dialect/ESI/runtime/tests/conftest.py | Pytest configuration with collect_ignore_glob patterns and helper utilities |
| lib/Dialect/ESI/runtime/tests/init.py | Package marker for tests directory |
| lib/Dialect/ESI/runtime/pyproject.toml | Added pytest optional dependencies (pytest, pycde, numpy) |
| integration_test/Dialect/ESI/runtime/type_construction.py | Removed old lit-based unit test (migrated to pytest) |
Migrate ESI runtime and PyCDE integration tests under the ESI runtime package with a pytest driver that uses the cosim library. Add pytest extras to the runtime package, update unit tests for current type APIs, add runtime root auto-detection with xdist guidance, and align generated loopback headers with the C++ test build.
266a192 to
3c0e166
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 33 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
lib/Dialect/ESI/runtime/cosim_dpi_server/esi-cosim.py:87
get_simulator()now raisesValueErrorfor unknown simulator names, which will bubble up and produce a Python stack trace from this CLI. Consider catchingValueErrorhere and printing a user-friendly message (including supported simulator names) and returning a non-zero exit code, similar to the previous behavior.
sources = SourceFiles(args.top)
sources.add_dir(Path(args.source))
sim = get_simulator(args.sim, sources, Path(args.rundir), args.debug)
if not args.no_compile:
rc = sim.compile()
if rc != 0:
return rc
return sim.run(args.inner_cmd[1:], gui=args.gui, server_only=args.server_only)
- Simplify MMIO region iteration in test_esi.py: use the mmio object already obtained from get_service_mmio() instead of iterating d.services. Also fix the incorrect type annotation on HWModule.services (List[AppID] -> List[Service]). - Remove unused run_cmd import in test_loopback_cpp.py. - Make warning_matcher default consistent: cosim_test() decorator now defaults to _DEFAULT_WARN_PATTERN, matching CosimPytestConfig. - Add tmpdir cleanup: temp directories created for compilation and test runs are cleaned up on completion unless debug=True. - Handle fork unavailability: gracefully skip tests with pytest.skip() on platforms where the fork start method is not available.
Remove esi-pytest-* temp dirs, obj_dir files, and runtime obj_dir that were accidentally staged.
Address review feedback: - Rename 'acc' variable/parameter to 'conn' for AcceleratorConnection throughout tests and the injection mechanism, since 'acc' is confusing. - Simplify _resolve_injected_params to use a single loop over parameters instead of separate checks followed by a loop.
- Remove pytest_cosim_plan.md (don't include plan doc) - Move esi-pytest-*/ gitignore pattern to runtime/.gitignore - Merge require_tool/require_env from tests/conftest.py into tests/integration/conftest.py; keep only collect_ignore_glob in parent - Convert test_loopback_query_info and test_loopback_query_hier into TestLoopbackQuery class with shared compilation - Replace _pytest_visible_signature + _copy_compiled_obj_dir with shared _is_injected_param helper and _copy_compiled_artifacts that copies the entire compile directory (works for Verilator, Questa, any backend) - Add sim stdout/stderr to _ChildResult; always pass logs through the result queue and include them in failure messages
Two bugs fixed: 1. test_esi.py: Restore the mmio_svc service lookup from d.services that was incorrectly collapsed to use the low-level conn.get_service_mmio() during the acc→conn rename. The low-level mmio object returns empty regions while the service-level one has the 5 expected regions. 2. pytest.py: Replace multiprocessing.Queue with multiprocessing.Pipe for child→parent result passing, and add os._exit(0) at the end of the forked child. The Queue's internal feeder thread caused hangs: either the child wouldn't exit (gRPC threads keeping the process alive) or the Queue data was lost when force-exiting. A Pipe writes synchronously (no feeder thread), so os._exit(0) after send()+close() is safe.
Without a timeout, reader.poll(timeout=None) blocks forever if a child process hangs. Under xdist parallel execution this causes non-deterministic hangs due to resource contention. Set a 120s default matching the lit integration test suite's CIRCT_INTEGRATION_TIMEOUT.
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.
Migrate ESI runtime and PyCDE integration tests under the ESI runtime
package with a pytest driver that uses the cosim library. Add pytest
extras to the runtime package, update unit tests for current type APIs,
add runtime root auto-detection with xdist guidance, and align generated
loopback headers with the C++ test build.