PyTest Migration #144
PyTest Migration #144
Conversation
diaconuccalin
left a comment
diaconuccalin
left a comment
There was a problem hiding this comment.
Great feature, fantastically useful! :)
One more comment other than the ones I've already left, maybe add the instructions you included in the PR description to a dedicated .md file in the DeeployTest directory, with a mention of it in the big README, for better documentation for future users (including myself :) ).
…test, fix generic runner
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/_runner-siracusa.yml:
- Around line 45-47: The pytest command places -v -n 4 between -m and its marker
argument, so pytest will treat -v as the marker; move the marker expression
immediately after -m so the command becomes something like: pytest -m "siracusa
and ${{ inputs.pytest-marker }}" -v -n 4 test_platforms.py (i.e., ensure -m is
immediately followed by "siracusa and ${{ inputs.pytest-marker }}" and then
place -v -n 4 afterwards).
🧹 Nitpick comments (2)
DeeployTest/README.md (1)
3-12: Consider improving markdown structure for consistency.Minor markdown linting issues flagged by static analysis:
- Heading jump from
#(h1) to###(h3) at line 5 - consider using##for major sections- Code blocks lack language specifiers (e.g., use
```bashfor shell commands,```textfor output)These don't affect functionality but improve rendering and accessibility.
📝 Example fix for heading structure
# How to use the DeeployTest PyTest Suite? -### Executing and Collecting Test Groups +## Executing and Collecting Test GroupsDeeployTest/test_platforms.py (1)
289-306: Consider prefixing unusedconfig_namewith underscore.The
config_namevariable is part of the parameter tuple for generating test IDs viaparam_id, but isn't used within the test body. This pattern repeats across all tiled test functions. Prefixing with underscore (_config_name) signals intentional non-use and silences the linter warning.♻️ Example fix (apply to all tiled tests)
def test_siracusa_tiled_kernels_l2_singlebuffer(test_params, deeploy_test_dir, toolchain, toolchain_dir, cmake_args, skipgen, skipsim) -> None: - test_name, l1, config_name = test_params + test_name, l1, _config_name = test_params
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/_runner-siracusa.yml.github/workflows/ci-platform-siracusa.ymlDeeployTest/README.mdDeeployTest/test_platforms.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T11:43:47.236Z
Learnt from: diaconuccalin
Repo: pulp-platform/Deeploy PR: 117
File: .github/workflows/ci-platform-siracusa.yml:57-60
Timestamp: 2025-09-24T11:43:47.236Z
Learning: In the Deeploy test system, test names in CI workflows correspond to directory names under DeeployTest/Tests/, not Python function names. The TestRunner class executes tests by passing directory paths via the `-t` argument, where each directory contains test configurations and definitions.
Applied to files:
.github/workflows/_runner-siracusa.ymlDeeployTest/README.md
🧬 Code graph analysis (1)
DeeployTest/test_platforms.py (2)
DeeployTest/testUtils/pytestRunner.py (2)
create_test_config(363-466)run_and_assert_test(469-488)DeeployTest/conftest.py (6)
deeploy_test_dir(86-88)toolchain(135-137)toolchain_dir(98-103)cmake_args(141-143)skipgen(123-125)skipsim(129-131)
🪛 markdownlint-cli2 (0.18.1)
DeeployTest/README.md
5-5: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
8-8: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.10)
DeeployTest/test_platforms.py
207-207: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
229-229: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
291-291: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
320-320: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
349-349: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
378-378: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
407-407: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
436-436: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
479-479: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
501-501: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
527-527: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
530-530: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
560-560: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
589-589: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
618-618: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
647-647: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
677-677: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
708-708: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: siracusa-models-tiled-l2-singlebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-models-tiled-l3-doublebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-models-tiled-l3-singlebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-neureka-models-tiled-singlebuffer-L3 / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-neureka-kernels-tiled-singlebuffer-L2 / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-neureka-models-tiled-doublebuffer-L3 / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-neureka-kernels-tiled-singlebuffer-L2-wmem / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-neureka-kernels-tiled-doublebuffer-L2 / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-neureka-models-tiled-doublebuffer-L3-wmem / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-models / test-runner-siracusa
- GitHub Check: siracusa-kernels / test-runner-siracusa
- GitHub Check: mempool-kernels / test-runner-mempool
- GitHub Check: mempool-models / test-runner-mempool
- GitHub Check: generic-models / test-runner-generic
- GitHub Check: generic-kernels / test-runner-generic
- GitHub Check: softhier-kernels / test-runner-softhier
- GitHub Check: docs
🔇 Additional comments (4)
.github/workflows/ci-platform-siracusa.yml (1)
32-46: LGTM!Clean migration to marker-based test selection. The
kernelsandmodelsmarkers will be combined withsiracusain the runner workflow to properly filter tests.DeeployTest/test_platforms.py (3)
38-59: LGTM - well-designed helper functions.The
generate_test_paramsandparam_idfunctions provide clean test parameterization with readable test IDs.
62-103: Clean platform configuration structure.The
PLATFORM_CONFIGSdictionary provides a well-organized central configuration for platforms. Note that Siracusa and Siracusa+Neureka configurations are handled inline in their respective test functions (withcoresparameter), which is appropriate given their additional complexity.
128-143: Well-structured test with proper marker and fixture usage.The test function pattern is clean: markers for selection, parameterization for test variations, fixture injection for configuration, and delegation to
create_test_config/run_and_assert_testfor execution. This pattern is consistently applied across all platform tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/_runner-siracusa.yml:
- Around line 14-20: The workflow uses hyphenated input names in expressions
(inputs.docker-image and inputs.pytest-marker) which GitHub Actions treats as
subtraction; update all occurrences of `${{ inputs.docker-image }}` and `${{
inputs.pytest-marker }}` to bracket notation `${{ inputs['docker-image'] }}` and
`${{ inputs['pytest-marker'] }}` respectively (search for these exact expression
strings in the template and replace them), ensuring every call site and
reference uses the bracket form so the workflow evaluates correctly.
🧹 Nitpick comments (1)
.github/workflows/_runner-siracusa.yml (1)
40-48: Preferenv+working-directoryfor marker passing; consider-n autoInlining
${{ inputs[...] }}into the bash command makes quoting/shell-parsing brittle; passing viaenvis safer and also reads cleaner. Also, hard-coding-n 4may under/over-utilize runners;-n autois usually a better default.Proposed refactor
- name: Run Test - run: | - cd DeeployTest - mkdir -p /app/.ccache - export CCACHE_DIR=/app/.ccache - - # Run tests using pytest markers - pytest test_platforms.py -v -n 4 -m "siracusa and ${{ inputs.pytest-marker }}" + working-directory: DeeployTest shell: bash + env: + CCACHE_DIR: /app/.ccache + PYTEST_MARKER: ${{ inputs['pytest-marker'] }} + run: | + set -euo pipefail + mkdir -p "${CCACHE_DIR}" + # Run tests using pytest markers + pytest test_platforms.py -v -n auto -m "siracusa and ${PYTEST_MARKER}"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/_runner-siracusa.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T11:43:47.236Z
Learnt from: diaconuccalin
Repo: pulp-platform/Deeploy PR: 117
File: .github/workflows/ci-platform-siracusa.yml:57-60
Timestamp: 2025-09-24T11:43:47.236Z
Learning: In the Deeploy test system, test names in CI workflows correspond to directory names under DeeployTest/Tests/, not Python function names. The TestRunner class executes tests by passing directory paths via the `-t` argument, where each directory contains test configurations and definitions.
Applied to files:
.github/workflows/_runner-siracusa.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: siracusa-neureka-models-tiled-doublebuffer-L3-wmem / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-neureka-models-tiled-singlebuffer-L3 / test-runner-siracusa-neureka-tiled
- GitHub Check: deeploy-internal-tests
- GitHub Check: siracusa-models / test-runner-siracusa
- GitHub Check: siracusa-kernels / test-runner-siracusa
- GitHub Check: linting
- GitHub Check: snitch-kernels-tiled-singlebuffer-L2 / test-runner-snitch-tiled
- GitHub Check: softhier-kernels / test-runner-softhier
- GitHub Check: chimera-kernels / test-runner-chimera
- GitHub Check: cortexm-kernels / test-runner-cortexm
- GitHub Check: cortexm-models / test-runner-cortexm
- GitHub Check: mempool-models / test-runner-mempool
- GitHub Check: mempool-kernels / test-runner-mempool
- GitHub Check: snitch-kernels / test-runner-snitch
- GitHub Check: generic-models / test-runner-generic
- GitHub Check: generic-kernels / test-runner-generic
- GitHub Check: docs
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@DeeployTest/deeployRunner_tiled_siracusa_w_neureka.py`:
- Around line 13-20: The create_config_from_args function currently parses
--neureka-wmem and --enable-3x3 via setup_parser but never forwards them to
testMVP.py; update create_config_from_args to append these flags to
gen_args_list like the existing tiling flags: check hasattr(args,
'neureka_wmem') and args.neureka_wmem then append "--neureka-wmem" to
gen_args_list, and likewise check hasattr(args, 'enable_3x3') and
args.enable_3x3 then append "--enable-3x3"; ensure these checks are placed
alongside the other gen_args_list population logic so the flags are passed
through when set.
In `@DeeployTest/testUtils/core/execution.py`:
- Around line 186-195: The code currently parses stdout/stderr with
parse_test_output but ignores subprocess return codes; update the block around
subprocess.run(cmd, ...) to check result.returncode and treat non-zero as a
failure: after capturing/printing output and calling
parse_test_output(result.stdout, result.stderr), if result.returncode != 0 then
set/augment the returned test_result (the variable test_result) to indicate
failure (e.g., mark success=false or increment error count), include the numeric
return code and result.stderr in the test_result details so callers can see the
process exit status; reference the variables/functions cmd, result,
parse_test_output and the test_result variable when making this change.
- Around line 68-69: Replace the assert on config.toolchain_install_dir with an
explicit runtime check that raises a RuntimeError if it's None; specifically, in
the function that currently does "assert config.toolchain_install_dir is not
None, 'LLVM_INSTALL_DIR environment variable not set'", change it to: if
config.toolchain_install_dir is None: raise RuntimeError("LLVM_INSTALL_DIR
environment variable not set") so it matches the existing RuntimeError style
used for CMake validation (see the existing RuntimeError usage around the CMake
validation block) and prevents passing "None" into CMake options.
In `@DeeployTest/testUtils/core/paths.py`:
- Around line 23-34: When a caller passes a relative base_dir it isn’t resolved
before comparing to test_path, so calls like test_path.relative_to(base_dir)
will incorrectly raise; update the branch that handles a provided base_dir
(where you currently do base_dir = Path(base_dir)) to resolve it to an absolute
path (e.g., base_dir = Path(base_dir).resolve()) before any use with
test_path.relative_to(...) or path joins; leave the existing resolved
script_path logic for the default branch untouched so base_dir is always
absolute when used.
In `@README.md`:
- Around line 77-79: Add a language tag to the fenced code block containing the
command that runs deeployRunner_tiled_siracusa.py; modify the block that
currently contains "python deeployRunner_tiled_siracusa.py -t
Tests/Kernels/Integer/MatMul/Regular --cores=8 --l1=16000" to use a language
identifier (e.g., bash) so the fence becomes ```bash and the command remains
inside, which fixes the markdownlint MD040 warning.
🧹 Nitpick comments (8)
DeeployTest/deeployRunner_tiled_snitch.py (1)
7-8: Minor: Import order inconsistency across runner scripts.Standard library imports (
sys) should conventionally come before third-party/local imports (testUtils.deeployRunner). This file imports them in reverse order compared todeeployRunner_generic.pyanddeeployRunner_tiled_siracusa.py.Suggested reorder
-from testUtils.deeployRunner import main import sys +from testUtils.deeployRunner import mainDeeployTest/deeployRunner_tiled_siracusa.py (1)
14-15: Consider: Inconsistent CLI argument naming across platforms.Siracusa runners use
--coreswhile Snitch/MemPool runners use--num-cores. Both work becausemain()checks for bothargs.coresandargs.num_cores, but this inconsistency may confuse users switching between platforms.Consider standardizing to one naming convention (e.g.,
--num-coreseverywhere) in a follow-up.DeeployTest/deeployRunner_snitch.py (1)
7-8: Minor: Import order inconsistency.Same as
deeployRunner_tiled_snitch.py- standard library imports should come before local imports.Suggested reorder
-from testUtils.deeployRunner import main import sys +from testUtils.deeployRunner import mainDeeployTest/deeployRunner_mempool.py (1)
8-9: Minor: Import order inconsistency.Same pattern as the Snitch runners - standard library imports should conventionally precede local imports.
Suggested reorder
-from testUtils.deeployRunner import main import sys +from testUtils.deeployRunner import mainDeeployTest/testUtils/core/__init__.py (1)
10-19: Optional: sort__all__to satisfy Ruff RUF022.♻️ Suggested ordering
__all__ = [ 'DeeployTestConfig', 'TestResult', - 'get_test_paths', - 'generate_network', - 'configure_cmake', 'build_binary', - 'run_simulation', + 'configure_cmake', + 'generate_network', + 'get_test_paths', 'run_complete_test', + 'run_simulation', ]DeeployTest/testUtils/pytestRunner.py (1)
101-114: Include stderr in assertion failures for better diagnostics.This makes CI failures easier to debug without reruns.
♻️ Suggested tweak
- assert result.success, (f"Test {test_name} failed with {result.error_count} errors out of {result.total_count}\n" - f"Output:\n{result.stdout}") + assert result.success, (f"Test {test_name} failed with {result.error_count} errors out of {result.total_count}\n" + f"Stdout:\n{result.stdout}\nStderr:\n{result.stderr}")DeeployTest/testUtils/deeployRunner.py (2)
391-402: Preserve traceback details when exceptions occur.Catching all exceptions without a traceback makes CI failures harder to diagnose.
♻️ Suggested tweak
- except Exception as e: - RED = '\033[91m' - RESET = '\033[0m' - print(f"\n{RED}✗ Test {config.test_name} FAILED with exception: {e}{RESET}") - return 1 + except Exception as e: + log.exception("Test runner failed") + RED = '\033[91m' + RESET = '\033[0m' + print(f"\n{RED}✗ Test {config.test_name} FAILED with exception: {e}{RESET}", file=sys.stderr) + return 1
349-349: Remove the extraneous f-string prefix.Keeps Ruff (F541) clean.
🧹 Suggested tweak
- print(f"Please use one of the following options:", file=sys.stderr) + print("Please use one of the following options:", file=sys.stderr)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
DeeployTest/deeployRunner_chimera.pyDeeployTest/deeployRunner_cortexm.pyDeeployTest/deeployRunner_generic.pyDeeployTest/deeployRunner_mempool.pyDeeployTest/deeployRunner_siracusa.pyDeeployTest/deeployRunner_snitch.pyDeeployTest/deeployRunner_softhier.pyDeeployTest/deeployRunner_tiled_siracusa.pyDeeployTest/deeployRunner_tiled_siracusa_w_neureka.pyDeeployTest/deeployRunner_tiled_snitch.pyDeeployTest/testUtils/core/__init__.pyDeeployTest/testUtils/core/config.pyDeeployTest/testUtils/core/execution.pyDeeployTest/testUtils/core/output_parser.pyDeeployTest/testUtils/core/paths.pyDeeployTest/testUtils/deeployRunner.pyDeeployTest/testUtils/pytestRunner.pyREADME.md
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: diaconuccalin
Repo: pulp-platform/Deeploy PR: 117
File: .github/workflows/ci-platform-siracusa.yml:57-60
Timestamp: 2025-09-24T11:43:47.236Z
Learning: In the Deeploy test system, test names in CI workflows correspond to directory names under DeeployTest/Tests/, not Python function names. The TestRunner class executes tests by passing directory paths via the `-t` argument, where each directory contains test configurations and definitions.
📚 Learning: 2025-09-24T11:43:47.236Z
Learnt from: diaconuccalin
Repo: pulp-platform/Deeploy PR: 117
File: .github/workflows/ci-platform-siracusa.yml:57-60
Timestamp: 2025-09-24T11:43:47.236Z
Learning: In the Deeploy test system, test names in CI workflows correspond to directory names under DeeployTest/Tests/, not Python function names. The TestRunner class executes tests by passing directory paths via the `-t` argument, where each directory contains test configurations and definitions.
Applied to files:
DeeployTest/deeployRunner_chimera.pyDeeployTest/deeployRunner_generic.pyDeeployTest/testUtils/core/execution.pyDeeployTest/testUtils/core/__init__.pyREADME.mdDeeployTest/deeployRunner_mempool.pyDeeployTest/deeployRunner_tiled_snitch.pyDeeployTest/deeployRunner_cortexm.pyDeeployTest/deeployRunner_softhier.pyDeeployTest/deeployRunner_tiled_siracusa_w_neureka.pyDeeployTest/testUtils/core/config.pyDeeployTest/testUtils/pytestRunner.pyDeeployTest/testUtils/deeployRunner.py
📚 Learning: 2025-09-09T15:58:06.454Z
Learnt from: Xeratec
Repo: pulp-platform/Deeploy PR: 105
File: Deeploy/Targets/PULPOpen/DMA/MchanDma.py:61-64
Timestamp: 2025-09-09T15:58:06.454Z
Learning: The tiling pipeline in Deeploy handles unit conversion and normalization through functions like _legalizeTransfers, ensuring that DMA implementations receive properly formatted transfer parameters without needing to perform manual element-to-byte conversions.
Applied to files:
README.md
🧬 Code graph analysis (13)
DeeployTest/deeployRunner_chimera.py (1)
DeeployTest/testUtils/deeployRunner.py (1)
main(292-402)
DeeployTest/deeployRunner_generic.py (1)
DeeployTest/testUtils/deeployRunner.py (1)
main(292-402)
DeeployTest/deeployRunner_siracusa.py (4)
DeeployTest/testUtils/deeployRunner.py (1)
main(292-402)DeeployTest/deeployRunner_snitch.py (1)
setup_parser(14-15)DeeployTest/deeployRunner_tiled_siracusa_w_neureka.py (1)
setup_parser(14-17)DeeployTest/deeployRunner_tiled_snitch.py (1)
setup_parser(14-15)
DeeployTest/testUtils/core/execution.py (2)
DeeployTest/testUtils/core/config.py (1)
DeeployTestConfig(11-33)DeeployTest/testUtils/core/output_parser.py (2)
TestResult(11-17)parse_test_output(20-49)
DeeployTest/deeployRunner_mempool.py (3)
DeeployTest/testUtils/deeployRunner.py (1)
main(292-402)DeeployTest/deeployRunner_chimera.py (1)
setup_parser(15-16)DeeployTest/deeployRunner_softhier.py (1)
setup_parser(15-17)
DeeployTest/deeployRunner_cortexm.py (1)
DeeployTest/testUtils/deeployRunner.py (1)
main(292-402)
DeeployTest/deeployRunner_softhier.py (2)
DeeployTest/deeployRunner_chimera.py (1)
setup_parser(15-16)DeeployTest/deeployRunner_mempool.py (1)
setup_parser(15-16)
DeeployTest/deeployRunner_tiled_siracusa_w_neureka.py (3)
DeeployTest/testUtils/deeployRunner.py (1)
main(292-402)DeeployTest/deeployRunner_siracusa.py (1)
setup_parser(13-14)DeeployTest/deeployRunner_tiled_siracusa.py (1)
setup_parser(14-15)
DeeployTest/deeployRunner_snitch.py (3)
DeeployTest/deeployRunner_siracusa.py (1)
setup_parser(13-14)DeeployTest/deeployRunner_tiled_siracusa_w_neureka.py (1)
setup_parser(14-17)DeeployTest/deeployRunner_tiled_snitch.py (1)
setup_parser(14-15)
DeeployTest/testUtils/core/config.py (1)
DeeployTest/conftest.py (1)
toolchain(135-137)
DeeployTest/deeployRunner_tiled_siracusa.py (2)
DeeployTest/deeployRunner_siracusa.py (1)
setup_parser(13-14)DeeployTest/deeployRunner_tiled_siracusa_w_neureka.py (1)
setup_parser(14-17)
DeeployTest/testUtils/pytestRunner.py (3)
DeeployTest/testUtils/core/config.py (1)
DeeployTestConfig(11-33)DeeployTest/testUtils/core/paths.py (1)
get_test_paths(11-48)DeeployTest/testUtils/core/execution.py (1)
run_complete_test(202-220)
DeeployTest/testUtils/deeployRunner.py (4)
DeeployTest/testUtils/core/config.py (1)
DeeployTestConfig(11-33)DeeployTest/testUtils/core/execution.py (1)
run_complete_test(202-220)DeeployTest/testUtils/core/paths.py (1)
get_test_paths(11-48)DeeployTest/conftest.py (3)
toolchain(135-137)skipgen(123-125)skipsim(129-131)
🪛 LanguageTool
README.md
[style] ~76-~76: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...he generated code for the Add test is very simple. However, this gets more complex when y...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.18.1)
README.md
77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.11)
DeeployTest/testUtils/core/execution.py
59-59: subprocess call: check for execution of untrusted input
(S603)
63-63: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Avoid specifying long messages outside the exception class
(TRY003)
111-111: subprocess call: check for execution of untrusted input
(S603)
115-115: Avoid specifying long messages outside the exception class
(TRY003)
136-136: subprocess call: check for execution of untrusted input
(S603)
140-140: Avoid specifying long messages outside the exception class
(TRY003)
155-155: Avoid specifying long messages outside the exception class
(TRY003)
186-186: subprocess call: check for execution of untrusted input
(S603)
197-197: f-string without any placeholders
Remove extraneous f prefix
(F541)
DeeployTest/testUtils/core/__init__.py
10-19: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
DeeployTest/testUtils/deeployRunner.py
349-349: f-string without any placeholders
Remove extraneous f prefix
(F541)
396-396: Consider moving this statement to an else block
(TRY300)
398-398: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: siracusa-kernels / test-runner-siracusa
- GitHub Check: generic-models / test-runner-generic
- GitHub Check: generic-kernels / test-runner-generic
- GitHub Check: mempool-kernels / test-runner-mempool
- GitHub Check: mempool-models / test-runner-mempool
- GitHub Check: cortexm-kernels / test-runner-cortexm
- GitHub Check: cortexm-models / test-runner-cortexm
- GitHub Check: snitch-kernels-tiled-singlebuffer-L2 / test-runner-snitch-tiled
- GitHub Check: siracusa-neureka-kernels-tiled-singlebuffer-L2-wmem / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-neureka-models-tiled-doublebuffer-L3-wmem / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-neureka-kernels-tiled-doublebuffer-L2 / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-neureka-kernels-tiled-singlebuffer-L2 / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-neureka-models-tiled-singlebuffer-L3 / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-neureka-models-tiled-doublebuffer-L3 / test-runner-siracusa-neureka-tiled
- GitHub Check: linting
- GitHub Check: snitch-kernels / test-runner-snitch
- GitHub Check: docs
🔇 Additional comments (24)
DeeployTest/deeployRunner_softhier.py (1)
12-20: LGTM — SoftHier wrapper cleanly delegates to the shared runner.
No concerns with the platform defaults or the added CLI arguments.DeeployTest/deeployRunner_chimera.py (1)
12-19: LGTM — Chimera wrapper matches the shared runner contract.DeeployTest/testUtils/core/output_parser.py (1)
10-49: LGTM — parsing logic is clear and robust for expected output patterns.DeeployTest/deeployRunner_cortexm.py (1)
11-12: LGTM — Cortex-M wrapper is minimal and correct.DeeployTest/deeployRunner_tiled_snitch.py (1)
11-18: LGTM!The tiled Snitch runner correctly wires the platform defaults (
Snitch,gvsoc, tiling enabled) and provides a--num-coresargument that aligns with the main function's handling ofnum_coresattribute for CMake configuration.DeeployTest/deeployRunner_generic.py (1)
1-12: LGTM!Clean and minimal implementation for the Generic platform runner. The script correctly delegates to the shared main function with appropriate defaults and proper import ordering.
DeeployTest/deeployRunner_tiled_siracusa.py (1)
11-18: LGTM!The tiled Siracusa runner correctly configures the platform with tiling enabled and provides the
--coresargument consistent with the non-tiled Siracusa runner.DeeployTest/deeployRunner_snitch.py (1)
11-18: LGTM!The non-tiled Snitch runner is correctly configured with
tiling_enabled=Falseand provides the--num-coresargument consistent with the tiled variant.DeeployTest/deeployRunner_mempool.py (1)
12-19: LGTM!The MemPool runner is correctly configured with
bansheesimulator and a higher default core count (16) appropriate for the MemPool platform's architecture. The docstring on line 6 is a nice addition for clarity.DeeployTest/deeployRunner_siracusa.py (1)
12-17: LGTM for the Siracusa runner entrypoint.DeeployTest/testUtils/core/config.py (1)
10-33: LGTM forDeeployTestConfig.DeeployTest/testUtils/core/execution.py (3)
17-64: LGTM for generation orchestration.Command assembly and failure handling are clear.
118-140: LGTM for the build step.Clear command construction and error handling.
202-219: LGTM for the end-to-end flow.The step orchestration is clear and consistent.
DeeployTest/testUtils/pytestRunner.py (2)
12-19: LGTM.Worker ID handling is straightforward.
22-98: No verification needed; test names from configuration files (e.g.,test_generic_config.py) are directory names withoutTests/prefix (e.g.,"Kernels/FP32/ReLU"). The function correctly prefixes them once withTests/, producing the expected pathTests/Kernels/FP32/ReLU. All call sites intest_platforms.pypass these unmodified test names from parametrization.DeeployTest/testUtils/deeployRunner.py (8)
20-21: LGTM.Simple helper with clear intent.
24-27: LGTM.Formatter setup is clean.
30-155: LGTM for the argument surface.Options and defaults look consistent with existing runner expectations.
157-170: LGTM.Logging configuration tied to verbosity is clear.
173-236: LGTM for config assembly.Path resolution and argument normalization are coherent.
239-253: LGTM.Output formatting is clear and readable.
255-289: LGTM.Configuration printout is comprehensive and well-structured.
405-406: LGTM.Standard CLI entry-point wiring.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@DeeployTest/testUtils/core/execution.py`:
- Line 197: The log.warning call uses an unnecessary f-string: replace the call
to log.warning(f"Could not parse error count from output") with a plain string
literal (log.warning("Could not parse error count from output")) to remove the
extraneous f-prefix in the code path where the parser logs an unparsable error
count.
In `@DeeployTest/testUtils/deeployRunner.py`:
- Around line 178-185: The build_dir is currently hardcoded to "build" which can
collide across pytest-xdist workers; modify the logic around build_dir in
DeeployTest/testUtils/deeployRunner.py to append the pytest worker id (from
os.environ.get("PYTEST_XDIST_WORKER","master")) so each worker uses a unique
directory (e.g., "build_master" or "build_{worker_id}"); update the variable
build_dir (and any subsequent uses) to construct Path(base_dir) /
f"TEST_{platform.upper()}" / f"build_{worker_id}" when worker_id != "master"
(and default to build_master for consistency) to match the approach used in
pytestRunner.py/get_pytest_worker_id.
♻️ Duplicate comments (2)
DeeployTest/testUtils/core/execution.py (2)
68-69: Replace the assert with an explicit runtime check.This concern was raised previously: asserts are stripped when running Python with
-O, which would causeconfig.toolchain_install_dirto remainNoneand be passed to CMake as"DTOOLCHAIN_INSTALL_DIR=None". The function already usesRuntimeErrorfor similar validation (line 73).Proposed fix
- assert config.toolchain_install_dir is not None, \ - "LLVM_INSTALL_DIR environment variable not set" + if config.toolchain_install_dir is None: + raise RuntimeError("LLVM_INSTALL_DIR environment variable not set")
186-199: Check subprocess return code to avoid false positives.This concern was raised previously: a non-zero exit status from the simulation subprocess could be reported as success if the output parsing happens to find zero errors. The return code should be checked before or after parsing.
Proposed fix
result = subprocess.run(cmd, capture_output = True, text = True, env = env) if result.stdout: print(result.stdout, end = '') if result.stderr: print(result.stderr, end = '', file = sys.stderr) # Parse output for error count and cycles test_result = parse_test_output(result.stdout, result.stderr) + # Check subprocess return code - non-zero indicates failure regardless of parsed output + if result.returncode != 0 and test_result.success: + log.warning(f"Simulation returned non-zero exit code {result.returncode} but output parsed as success") + test_result = TestResult( + success = False, + error_count = test_result.error_count, + total_count = test_result.total_count, + stdout = test_result.stdout, + stderr = test_result.stderr, + runtime_cycles = test_result.runtime_cycles, + ) + if not test_result.success and test_result.error_count == -1: - log.warning(f"Could not parse error count from output") + log.warning("Could not parse error count from output") return test_result
🧹 Nitpick comments (2)
DeeployTest/deeployRunner_snitch.py (1)
1-7: Consider adding a module docstring for consistency.Other platform runners (e.g.,
deeployRunner_softhier.py,deeployRunner_chimera.py) include a module docstring like"""Deeploy runner for <Platform> platform.""". Adding one here would maintain consistency across the codebase.Suggested fix
# SPDX-License-Identifier: Apache-2.0 +"""Deeploy runner for Snitch platform.""" from testUtils.deeployRunner import mainDeeployTest/testUtils/deeployRunner.py (1)
395-406: Preserve stack traces on failures.Catching
Exceptionwithout logging the traceback makes failures harder to debug. Consider logging the full exception (or re-raising when verbose/debug).🪵 Minimal logging improvement
- except Exception as e: + except Exception as e: RED = '\033[91m' RESET = '\033[0m' - print(f"\n{RED}✗ Test {config.test_name} FAILED with exception: {e}{RESET}") + log.exception("Test %s failed with exception", config.test_name) + print(f"\n{RED}✗ Test {config.test_name} FAILED with exception: {e}{RESET}") return 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
DeeployTest/deeployRunner_chimera.pyDeeployTest/deeployRunner_cortexm.pyDeeployTest/deeployRunner_generic.pyDeeployTest/deeployRunner_mempool.pyDeeployTest/deeployRunner_siracusa.pyDeeployTest/deeployRunner_snitch.pyDeeployTest/deeployRunner_softhier.pyDeeployTest/deeployRunner_tiled_siracusa.pyDeeployTest/deeployRunner_tiled_siracusa_w_neureka.pyDeeployTest/deeployRunner_tiled_snitch.pyDeeployTest/testUtils/core/execution.pyDeeployTest/testUtils/core/output_parser.pyDeeployTest/testUtils/deeployRunner.py
🚧 Files skipped from review as they are similar to previous changes (7)
- DeeployTest/deeployRunner_tiled_siracusa.py
- DeeployTest/deeployRunner_tiled_snitch.py
- DeeployTest/deeployRunner_cortexm.py
- DeeployTest/deeployRunner_generic.py
- DeeployTest/deeployRunner_tiled_siracusa_w_neureka.py
- DeeployTest/deeployRunner_siracusa.py
- DeeployTest/deeployRunner_mempool.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T11:43:47.236Z
Learnt from: diaconuccalin
Repo: pulp-platform/Deeploy PR: 117
File: .github/workflows/ci-platform-siracusa.yml:57-60
Timestamp: 2025-09-24T11:43:47.236Z
Learning: In the Deeploy test system, test names in CI workflows correspond to directory names under DeeployTest/Tests/, not Python function names. The TestRunner class executes tests by passing directory paths via the `-t` argument, where each directory contains test configurations and definitions.
Applied to files:
DeeployTest/testUtils/core/execution.pyDeeployTest/testUtils/deeployRunner.pyDeeployTest/deeployRunner_softhier.pyDeeployTest/deeployRunner_chimera.pyDeeployTest/deeployRunner_snitch.py
🧬 Code graph analysis (5)
DeeployTest/testUtils/core/execution.py (5)
DeeployTest/testUtils/core/config.py (1)
DeeployTestConfig(11-33)DeeployTest/testUtils/core/output_parser.py (2)
TestResult(11-17)parse_test_output(20-49)DeeployTest/testUtils/testRunner.py (1)
run(325-336)DeeployTest/conftest.py (3)
toolchain(135-137)skipgen(123-125)skipsim(129-131)Deeploy/DeeployTypes.py (1)
copy(1016-1020)
DeeployTest/testUtils/deeployRunner.py (4)
DeeployTest/testUtils/core/config.py (1)
DeeployTestConfig(11-33)DeeployTest/testUtils/core/execution.py (1)
run_complete_test(202-220)DeeployTest/testUtils/core/paths.py (1)
get_test_paths(11-48)DeeployTest/conftest.py (3)
toolchain(135-137)skipgen(123-125)skipsim(129-131)
DeeployTest/deeployRunner_softhier.py (8)
DeeployTest/testUtils/deeployRunner.py (1)
main(294-406)DeeployTest/deeployRunner_chimera.py (1)
setup_parser(13-14)DeeployTest/deeployRunner_mempool.py (1)
setup_parser(13-18)DeeployTest/deeployRunner_siracusa.py (1)
setup_parser(12-13)DeeployTest/deeployRunner_snitch.py (1)
setup_parser(12-17)DeeployTest/deeployRunner_tiled_siracusa.py (1)
setup_parser(12-13)DeeployTest/deeployRunner_tiled_siracusa_w_neureka.py (1)
setup_parser(12-15)DeeployTest/deeployRunner_tiled_snitch.py (1)
setup_parser(12-17)
DeeployTest/deeployRunner_chimera.py (7)
DeeployTest/deeployRunner_mempool.py (1)
setup_parser(13-18)DeeployTest/deeployRunner_siracusa.py (1)
setup_parser(12-13)DeeployTest/deeployRunner_snitch.py (1)
setup_parser(12-17)DeeployTest/deeployRunner_softhier.py (1)
setup_parser(13-19)DeeployTest/deeployRunner_tiled_siracusa.py (1)
setup_parser(12-13)DeeployTest/deeployRunner_tiled_siracusa_w_neureka.py (1)
setup_parser(12-15)DeeployTest/deeployRunner_tiled_snitch.py (1)
setup_parser(12-17)
DeeployTest/deeployRunner_snitch.py (8)
DeeployTest/testUtils/deeployRunner.py (1)
main(294-406)DeeployTest/deeployRunner_chimera.py (1)
setup_parser(13-14)DeeployTest/deeployRunner_mempool.py (1)
setup_parser(13-18)DeeployTest/deeployRunner_siracusa.py (1)
setup_parser(12-13)DeeployTest/deeployRunner_softhier.py (1)
setup_parser(13-19)DeeployTest/deeployRunner_tiled_siracusa.py (1)
setup_parser(12-13)DeeployTest/deeployRunner_tiled_siracusa_w_neureka.py (1)
setup_parser(12-15)DeeployTest/deeployRunner_tiled_snitch.py (1)
setup_parser(12-17)
🪛 Ruff (0.14.11)
DeeployTest/testUtils/core/execution.py
59-59: subprocess call: check for execution of untrusted input
(S603)
63-63: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Avoid specifying long messages outside the exception class
(TRY003)
111-111: subprocess call: check for execution of untrusted input
(S603)
115-115: Avoid specifying long messages outside the exception class
(TRY003)
136-136: subprocess call: check for execution of untrusted input
(S603)
140-140: Avoid specifying long messages outside the exception class
(TRY003)
155-155: Avoid specifying long messages outside the exception class
(TRY003)
186-186: subprocess call: check for execution of untrusted input
(S603)
197-197: f-string without any placeholders
Remove extraneous f prefix
(F541)
DeeployTest/testUtils/deeployRunner.py
352-352: f-string without any placeholders
Remove extraneous f prefix
(F541)
400-400: Consider moving this statement to an else block
(TRY300)
402-402: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: chimera-kernels / test-runner-chimera
- GitHub Check: linting
- GitHub Check: softhier-kernels / test-runner-softhier
- GitHub Check: deeploy-internal-tests
- GitHub Check: build-deeploy
- GitHub Check: snitch-kernels-tiled-singlebuffer-L2 / test-runner-snitch-tiled
- GitHub Check: siracusa-kernels / test-runner-siracusa
- GitHub Check: siracusa-models / test-runner-siracusa
- GitHub Check: siracusa-models-tiled-l2-singlebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-kernels-tiled-l2-doublebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-models-tiled-l2-doublebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-models-tiled-l3-singlebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-kernels-tiled-l2-singlebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-models-tiled-l3-doublebuffer / test-runner-siracusa-tiled
- GitHub Check: cortexm-kernels / test-runner-cortexm
- GitHub Check: cortexm-models / test-runner-cortexm
🔇 Additional comments (7)
DeeployTest/deeployRunner_softhier.py (1)
1-25: LGTM!The SoftHier runner correctly follows the established pattern for platform-specific runners. The
--num-clustersand--coresarguments are properly configured and will be handled by the main function's CMake argument extraction logic.DeeployTest/deeployRunner_chimera.py (1)
1-20: LGTM!The Chimera runner follows the established pattern and is consistent with similar runners like Siracusa.
DeeployTest/testUtils/core/output_parser.py (1)
10-49: LGTM!The
TestResultdataclass andparse_test_outputfunction are well-designed. The fallback behavior of treating unparseable output as a failure (with sentinel values-1) is a safe approach.DeeployTest/testUtils/core/execution.py (3)
17-63: LGTM!The
generate_networkfunction properly handles the skip case, constructs the command correctly, and raisesRuntimeErroron failure.
118-140: LGTM!The
build_binaryfunction properly handles the build process and raisesRuntimeErroron failure.
202-220: LGTM!The
run_complete_testfunction properly orchestrates the test flow and propagates the result.DeeployTest/testUtils/deeployRunner.py (1)
382-389: The concern aboutNonevalues is not applicable—all platform-specific arguments have explicit non-Nonedefaults.All platform-specific runners define these arguments through
argparsewith explicit defaults:num_coresdefaults to 8 or 16,num_clustersdefaults to 1, andcoresdefaults to 8. Sinceargparseenforcestype=inton all these arguments and provides non-Nonedefaults, the attributes will never beNoneat runtime. Thehasattr()checks are correctly used here to determine whether optional platform-specific attributes exist on the args object; they don't need additional value guards.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@runwangdl I implemented your feedback, and we now have a CLI interface for developers. The logic behind is centralized and used also by the pytest suite. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@DeeployTest/testUtils/deeployRunner.py`:
- Line 359: Remove the unnecessary f-string prefix on the print call that
currently reads print(f"Please use one of the following options:", file =
sys.stderr); change it to a normal string literal (print("Please use one of the
following options:", file=sys.stderr)) and tidy the keyword argument spacing
(use file=sys.stderr) so no f-string is used for a string with no placeholders.
🧹 Nitpick comments (1)
DeeployTest/testUtils/deeployRunner.py (1)
409-413: Consider catching more specific exceptions.Catching bare
Exceptioncan mask unexpected bugs. While this is common in CLI entry points for graceful error handling, consider catching more specific exception types (e.g.,RuntimeError,OSError,subprocess.CalledProcessError) that the underlyingrun_complete_testmight raise.Proposed refinement
- except Exception as e: + except (RuntimeError, OSError, KeyboardInterrupt) as e: RED = '\033[91m' RESET = '\033[0m' print(f"\n{RED}✗ Test {config.test_name} FAILED with exception: {e}{RESET}") return 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
DeeployTest/testUtils/deeployRunner.pyDeeployTest/testUtils/pytestRunner.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: diaconuccalin
Repo: pulp-platform/Deeploy PR: 117
File: .github/workflows/ci-platform-siracusa.yml:57-60
Timestamp: 2025-09-24T11:43:47.236Z
Learning: In the Deeploy test system, test names in CI workflows correspond to directory names under DeeployTest/Tests/, not Python function names. The TestRunner class executes tests by passing directory paths via the `-t` argument, where each directory contains test configurations and definitions.
📚 Learning: 2025-09-24T11:43:47.236Z
Learnt from: diaconuccalin
Repo: pulp-platform/Deeploy PR: 117
File: .github/workflows/ci-platform-siracusa.yml:57-60
Timestamp: 2025-09-24T11:43:47.236Z
Learning: In the Deeploy test system, test names in CI workflows correspond to directory names under DeeployTest/Tests/, not Python function names. The TestRunner class executes tests by passing directory paths via the `-t` argument, where each directory contains test configurations and definitions.
Applied to files:
DeeployTest/testUtils/deeployRunner.pyDeeployTest/testUtils/pytestRunner.py
🧬 Code graph analysis (2)
DeeployTest/testUtils/deeployRunner.py (4)
DeeployTest/testUtils/core/config.py (1)
DeeployTestConfig(11-33)DeeployTest/testUtils/core/execution.py (1)
run_complete_test(202-220)DeeployTest/testUtils/core/paths.py (1)
get_test_paths(11-48)DeeployTest/conftest.py (3)
toolchain(135-137)skipgen(123-125)skipsim(129-131)
DeeployTest/testUtils/pytestRunner.py (4)
DeeployTest/testUtils/core/config.py (1)
DeeployTestConfig(11-33)DeeployTest/testUtils/core/execution.py (4)
build_binary(118-140)configure_cmake(66-115)run_complete_test(202-220)run_simulation(143-199)DeeployTest/testUtils/core/paths.py (1)
get_test_paths(11-48)DeeployTest/conftest.py (6)
deeploy_test_dir(86-88)toolchain(135-137)toolchain_dir(98-103)cmake_args(141-143)skipgen(123-125)skipsim(129-131)
🪛 Ruff (0.14.11)
DeeployTest/testUtils/deeployRunner.py
359-359: f-string without any placeholders
Remove extraneous f prefix
(F541)
407-407: Consider moving this statement to an else block
(TRY300)
409-409: Do not catch blind exception: Exception
(BLE001)
DeeployTest/testUtils/pytestRunner.py
11-18: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: snitch-kernels / test-runner-snitch
- GitHub Check: siracusa-models / test-runner-siracusa
- GitHub Check: siracusa-kernels / test-runner-siracusa
- GitHub Check: deeploy-internal-tests
- GitHub Check: mempool-models / test-runner-mempool
- GitHub Check: mempool-kernels / test-runner-mempool
- GitHub Check: linting
- GitHub Check: softhier-kernels / test-runner-softhier
- GitHub Check: siracusa-kernels-tiled-l2-doublebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-kernels-tiled-l2-singlebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-models-tiled-l3-singlebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-models-tiled-l3-doublebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-models-tiled-l2-singlebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-models-tiled-l2-doublebuffer / test-runner-siracusa-tiled
- GitHub Check: cortexm-models / test-runner-cortexm
- GitHub Check: cortexm-kernels / test-runner-cortexm
- GitHub Check: docs
🔇 Additional comments (9)
DeeployTest/testUtils/pytestRunner.py (4)
1-19: LGTM on imports and module structure.The imports are correct and the module exposes a clean public API. Note: Ruff flags that
__all__is unsorted, but this is a minor style consideration.
21-28: LGTM!Clean utility for detecting pytest-xdist worker ID with appropriate default for non-parallel execution.
31-107: LGTM!The configuration builder correctly:
- Derives paths using
get_test_paths- Isolates build directories per pytest-xdist worker
- Conditionally appends tiling and platform-specific arguments only when they differ from defaults
This provides clean separation between test orchestration and the core execution layer.
110-123: LGTM!The dual assertion strategy is appropriate:
- First assertion ensures the overall test succeeded
- Second assertion (when
error_count >= 0) provides additional verification that no errors were loggedThis correctly handles cases where
error_count = -1indicates output parsing failure (as documented inrun_simulation).DeeployTest/testUtils/deeployRunner.py (5)
1-23: LGTM on imports and helper function.The
cmake_strhelper correctly handles unicode escaping for CMake arguments.
26-171: LGTM!The argument parser is well-structured with:
- Conditional tiling arguments based on the
tiling_argumentsflag- Appropriate verbosity-to-log-level mapping
- Clear help text for all options
174-245: LGTM! Worker isolation fix is correctly implemented.Lines 186-191 now properly handle pytest-xdist worker isolation with worker-specific build directories, consistent with the pattern in
pytestRunner.py. This resolves the previous concern about parallel test collisions.
248-298: LGTM!The colored output functions provide clear, informative feedback for CLI usage. The configuration display comprehensively shows all relevant test parameters.
416-417: LGTM!Standard CLI entry point pattern.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@DeeployTest/conftest.py`:
- Around line 91-94: The deeploy_test_dir fixture returns a pathlib.Path but
create_test_config currently types its deeploy_test_dir parameter as str; update
the type hints to accept both Path and str (e.g., use Union[str, Path] or str |
Path) on the create_test_config parameter (and any related functions) so type
checkers align with the fixture, and ensure get_test_paths (already converting
to Path) remains unchanged.
🧹 Nitpick comments (4)
DeeployTest/testUtils/deeployRunner.py (1)
411-422: Exception handling in main entry point.The broad
Exceptioncatch on line 418 is acceptable for a CLI entry point where you want to ensure a clean error message and proper exit code rather than a stack trace. However, consider logging the full traceback at debug level for troubleshooting.💡 Optional: Log traceback for debugging
except Exception as e: RED = '\033[91m' RESET = '\033[0m' print(f"\n{RED}✗ Test {config.test_name} FAILED with exception: {e}{RESET}") + log.debug(f"Exception traceback:", exc_info=True) return 1DeeployTest/testUtils/pytestRunner.py (1)
11-18: Consider sorting__all__for consistency.The
__all__list is not alphabetically sorted. This is a minor style preference flagged by Ruff (RUF022).💡 Optional: Sort __all__ alphabetically
__all__ = [ + 'build_binary', + 'configure_cmake', + 'create_test_config', 'get_worker_id', - 'create_test_config', 'run_and_assert_test', - 'build_binary', - 'configure_cmake', 'run_simulation', ]DeeployTest/test_platforms.py (2)
63-103: PLATFORM_CONFIGS provides good centralization.The configuration dictionary centralizes platform settings. Note that Siracusa and Neureka tests hardcode their platform/simulator values directly rather than using this dict - consider extending
PLATFORM_CONFIGSto include them for consistency.
295-295: Prefix unusedconfig_namewith underscore.The
config_namevariable is unpacked but never used in the function body. This pattern appears in multiple tiled test functions. Prefix with_to indicate intentional non-use.💡 Proposed fix for all occurrences
- test_name, l1, config_name = test_params + test_name, l1, _config_name = test_paramsApply this change to all tiled test functions where
config_nameis unpacked but unused:
- Lines 295, 324, 353, 382, 411, 440, 531, 564, 593, 622, 651, 681, 712
runwangdl
left a comment
runwangdl
left a comment
There was a problem hiding this comment.
Looks good to me! Thank you for your contribution.
diaconuccalin
left a comment
diaconuccalin
left a comment
There was a problem hiding this comment.
Great new features, looks like lots of work so thank you. LGTM
3 participants
We used to write the tests directly in the GitHub Action YAML files; this is not a good practice, as it is hard to run several tests locally. PyTest is a well-known test framework that allows decoupling the testing from the GitHub CI, so users can easily run all tests for any platform locally if needed.
Currently, I use PyTest only for the Generic platform. I'd like a first round of review from @Xeratec or @diaconuccalin before moving forward. Moving the CI for other platforms will be done in a similar way.All platforms and internal deeploy tests are now moved to the PyTest suite and the CI is only using the PyTest suite.
Additionally, the PyTest suite can run tests in parallel and generate one build folder per worker to prevent conflicts.
Tagging @haugoug as he's interested in this feature.
To run all the tests of the generic platform with 8 parallel threads, you can do
You can select a specific marker with
-m, like only the kernels, you can do:To list all the captured tests for a given expression, you can do:
To execute a specific test, you can use:
You can also use an expression to filter tests, for instance the following command execute all test whose name contain "Adder":
I summarized the important points to use the PyTest suite in a README in the
DeeployTestfolder.Added
pytestandpytest-xdistas dependencies of Deeploy.pytest.inifor the global configuration of PyTest for the project.conftest.pyto define CLI args for PyTest for the whole project, it also defines a set of global fixtures and markers.pytestRunner.pycontains helper functions and fixtures for the whole project.test_platforms.pylists the E2E tests and sorts them into marked categories (per platform and per kernel/model).Changed
Fixed
nvidia-pyindexwas broken as it now tries to build the wheel to respect the new policy on packages usingpyproject. Instead of installing this package, we just add thehttps://pypi.ngc.nvidia.comchannel to the pip config file.PR Merge Checklist
develcommit and pointing todevel.CHANGELOG.mdfile has been updated.