Adds detection and reporting for ISA extensions that are implemented but have no selected tests.#1315
Adds detection and reporting for ISA extensions that are implemented but have no selected tests.#1315MainakSil wants to merge 6 commits intoriscv:act4from
Conversation
Agent-Logs-Url: https://github.com/MainakSil/riscv-arch-test/sessions/348fab9e-3289-4b7e-8fcc-79bc6dfb4116 Co-authored-by: MainakSil <145980094+MainakSil@users.noreply.github.com>
…xtensions Report implemented extensions that resolve to zero selected tests in ACT4
There was a problem hiding this comment.
Pull request overview
This PR adds a visibility check in ACT to detect ISA extensions reported as implemented by the UDB config but for which no applicable tests were selected, and emits a warning to help surface potential coverage gaps.
Changes:
- Added
get_untested_implemented_extensions()to compute implemented extensions that have no selected tests. - Integrated the new check into the main
run_actflow to emit a warning when uncovered implemented extensions are detected (when--extensions=all).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| framework/src/act/select_tests.py | Adds helper to derive “implemented but untested” extension names from selected test metadata. |
| framework/src/act/act.py | Calls the helper after test selection and prints a warning listing uncovered implemented extensions. |
| excluded_extensions = {ext.strip() for ext in exclude.split(",") if ext.strip()} | ||
| for config_file in config_files: | ||
| # Load configuration |
There was a problem hiding this comment.
excluded_extensions is derived from the CLI --exclude string, which generate_test_dict() applies as a test directory filter (it compares against test_file.parent.name, e.g. Sv). Passing this set into get_untested_implemented_extensions() (which compares against ISA extension tokens like Sv39, Zba, etc.) means excludes like Sv won’t suppress warnings for implemented Sv32/Sv39/..., leading to false-positive warnings. Consider translating excluded directory names into ISA extension names (e.g., treat an excluded prefix like Sv as excluding any implemented extension starting with Sv), or only applying this filter when the excluded value is known to be in the same namespace as implemented_extensions.
There was a problem hiding this comment.
Seems like that didn't work.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Mainak Sil <mainaksil0@gmail.com>
jordancarlin
left a comment
There was a problem hiding this comment.
Thanks for working on this! One comment below, but otherwise this looks pretty good.
| if extensions == "all": | ||
| untested_extensions = get_untested_implemented_extensions( | ||
| selected_tests, | ||
| implemented_extensions, | ||
| include_priv_tests=config.include_priv_tests, | ||
| excluded_extensions=excluded_extensions, | ||
| ) | ||
| if untested_extensions: | ||
| print( | ||
| f"Warning: {config.name}: no applicable tests selected for implemented extension(s): " | ||
| f"{', '.join(untested_extensions)}. Tests may be missing, excluded, or " | ||
| "otherwise inapplicable for the current configuration.", | ||
| file=sys.stderr, | ||
| ) |
There was a problem hiding this comment.
I don't think we want to limit this to just when extensions == "all". If someone limits the list of extensions, we should still warn for extensions that we don't support. Seems like the simplest implementation might be to move this call into select_tests, which is probably a better place for it anyway since this is a warning that comes from the test selection process.
Added warning for untested implemented extensions in select tests. Signed-off-by: Mainak Sil <mainaksil0@gmail.com>
Removed untested extensions warning logic from test selection process. Signed-off-by: Mainak Sil <mainaksil0@gmail.com>
| selected_tests = select_tests( | ||
| full_test_dict, implemented_extensions, config_params, include_priv_tests=config.include_priv_tests | ||
| full_test_dict, | ||
| implemented_extensions, | ||
| config_params, | ||
| include_priv_tests=config.include_priv_tests, | ||
| excluded_extensions=excluded_extensions, | ||
| ) | ||
| mxlen = config_params["MXLEN"] | ||
| if not isinstance(mxlen, int): | ||
| raise TypeError(f"MXLEN must be an integer, got {type(mxlen)}: {mxlen!r}") | ||
|
|
||
| config_names.append(config.name) | ||
| tasks.extend( | ||
| generate_build_plan( | ||
| config, | ||
| mxlen, | ||
| selected_tests, |
There was a problem hiding this comment.
act.py now emits an untested-extension warning (lines 119-132), but select_tests() also emits a similar warning unconditionally (and act.py doesn’t pass excluded_extensions into select_tests()). This leads to duplicate output and the first warning ignoring --exclude. Recommended: centralize the warning in one place (preferably act.py, since it knows extensions == 'all'), and ensure the exclude list is applied consistently.
| # Warn about implemented extensions with no tests | ||
| untested_extensions = get_untested_implemented_extensions( | ||
| selected_tests, | ||
| implemented_extensions, | ||
| include_priv_tests=include_priv_tests, | ||
| excluded_extensions=excluded_extensions, | ||
| ) | ||
|
|
||
| if untested_extensions: | ||
| print( | ||
| "Warning: no applicable tests selected for implemented extension(s): " | ||
| + ", ".join(untested_extensions), | ||
| file=sys.stderr, | ||
| ) |
There was a problem hiding this comment.
select_tests() now prints a warning as a side-effect. This causes duplicate/contradictory warnings because act.py also prints a warning (and only does so when extensions == 'all'). Consider removing the warning/printing from select_tests() and letting callers handle reporting via get_untested_implemented_extensions(), or add an explicit warn_untested_extensions flag so callers can control when/if it prints (and pass through excluded_extensions).
| excluded_extensions = {ext.strip() for ext in exclude.split(",") if ext.strip()} | ||
| for config_file in config_files: | ||
| # Load configuration | ||
| config = load_config(config_file) | ||
| config_dir = workdir / config.udb_config.stem | ||
| config_dir.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| # UDB integration | ||
| generate_udb_files(config.udb_config, config_dir) | ||
| implemented_extensions = get_implemented_extensions(config_dir / "extensions.txt") | ||
| config_params = get_config_params(config.udb_config) | ||
|
|
||
| # Select tests for config | ||
| selected_tests = select_tests( | ||
| full_test_dict, implemented_extensions, config_params, include_priv_tests=config.include_priv_tests | ||
| full_test_dict, | ||
| implemented_extensions, | ||
| config_params, | ||
| include_priv_tests=config.include_priv_tests, | ||
| excluded_extensions=excluded_extensions, | ||
| ) | ||
| mxlen = config_params["MXLEN"] | ||
| if not isinstance(mxlen, int): | ||
| raise TypeError(f"MXLEN must be an integer, got {type(mxlen)}: {mxlen!r}") |
There was a problem hiding this comment.
The excluded_extensions filter is derived from the CLI --exclude list, which filters by test directory name (e.g. Sv, ZfaD). In get_untested_implemented_extensions() it’s compared against implemented ISA extension tokens; this works for prefix cases like Sv*, but fails for composite dirs like ZfaD (tests require Zfa/D/F, so excluding ZfaD won’t suppress warnings for Zfa etc.). Either translate excluded test directories into the corresponding REQUIRED_EXTENSIONS tokens (by reading metadata of excluded tests), or only apply this suppression when the excluded value is known to be an ISA extension token.
MainakSil
left a comment
There was a problem hiding this comment.
Fixed the duplicated select_tests() call and ensured the warning is only emitted from select_tests, as suggested. Removed warning handling from act.py to avoid duplication. Let me know if you'd prefer the warning to be handled at a different layer.
| if untested_extensions: | ||
| print( | ||
| "Warning: no applicable tests selected for implemented extension(s): " | ||
| + ", ".join(untested_extensions), | ||
| file=sys.stderr, | ||
| ) |
There was a problem hiding this comment.
Seems like it would be cleaner to move the warning printing into get_untested_implemented_extensions and then rename that function something like check_untested_implemented_extensions. That way there is no need to pass the list back and forth.
| if excluded_extensions: | ||
| untested_extensions = { | ||
| ext for ext in untested_extensions | ||
| if not _is_excluded_extension(ext, excluded_extensions) | ||
| } |
There was a problem hiding this comment.
Let's drop the excluded extensions check in this function. If an extension is excluded it is valid to print a warning saying that extension is not tested. The EXCLUDED_EXTENSIONS list can be hidden in a Makefile and might not be obvious to users.
Fixes #1041
Motivation
Previously, when an extension (e.g., A-extension in RV32E) was implemented but no tests existed, the framework would silently skip it. This could mislead users into assuming full test coverage.
Changes
get_untested_implemented_extensions()to detect uncovered extensionsNotes
Impact