Split check_test_cases.py and outcome_analysis.py #9668

gilles-peskine-arm · 2024-10-03T17:31:48Z

Split check_test_cases.py into test case collection (collect_test_cases.py, used by outcome analysis) and the checks proper. Move both parts to the framework.
Split analyze_outcomes.py into code (outcome_analysis.py, moved to the framework) and data (analyze_outcomes.py, stays in its branch).
Upgrade mypy, for the sake of a new feature in outcome_analysis.py.
Prepare to require all test cases to be executed, i.e. to enforcing full coverage. But for now, keep it as a warning in this branch. To be continued in Test cases not executed: switch to enforcement mode #9593.

PR checklist

changelog not required because: test stuff only
development PR here
framework PR provided Framework: Split check_test_cases.py and outcome_analysis.py mbedtls-framework#55
3.6 PR provided Backport 3.6: Split check_test_cases.py and outcome_analysis.py #9669
2.28 PR provided # | not required because: no framework in 2.28, and the improvements to outcome analysis aren't worth backporting at this stage
tests provided

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Move the test case collection code out of check_test_cases.py and into its own module. This allows outcome analysis to depend only on the new module and not on check_test_cases.py. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

…mes.py Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

The ignore list for coverage only has two test cases out of ~10000 that are currently reported as not executed. This is a drop in the sea and not useful. Remove them so that the class can be used generically. A follow-up will construct a comprehensive ignore list. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Use different names for task name, a task class and a task instance. The interpreter doesn't care, but it's less confusing for both humans and type checkers. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Always have tasks_list be a list, not potentially some fancier iterable. Bypass mypy's somewhat legitimate complaint about REFERENCE and DRIVER in task_class: they could potentially be instance attributes, but we rely on them being class attributes. Python does normally guarantee their existence as class attributes (unless a derived class explicitly deletes them), but they could be overridden by an instance attribute; that's just something we don't do, so the class attribute's value is legitimate. We can't expect mypy to know that, so work around its complaint. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Place the code of outcome analysis (auxiliary functions, tasks, command line entry point) into a separate module, which will be moved to the version-independent framework repository so that it can be shared between maintained branches. Keep the branch-specific list of driver components and ignore lists in the per-repository script. We keep the executable script at `tests/scripts/analyze_outcomes.py`. It's simpler that way, because that path is hard-coded in CI scripts. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

eleuzi01

LGTM

mpg

Looks good to me. Only two minor comments.

tests/scripts/analyze_outcomes.py

scripts/ci.requirements.txt

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Move `collect_test_cases.py` (split from `check_test_cases.py`), `check_test_cases.py`, and `outcome_analysis.py` (split from `analyze_outcomes.py`) to the framework repository. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Upgrade mypy to 0.971, which is the last version that supports Python 3.6 (the oldest Python version that we currently run on the CI). This fixes the error ``` framework/scripts/mbedtls_framework/outcome_analysis.py:119: error: Incompatible return value type (got "IO[Any]", expected "TextIO") framework/scripts/mbedtls_framework/outcome_analysis.py:121: error: Incompatible return value type (got "IO[Any]", expected "TextIO") ``` As far as I can tell the fix is python/mypy#9275 which was released in mypy 0.940. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

mypy >=0.960 rejects macro_collector.py. Mbed-TLS/mbedtls-framework#50 We currently need mypy >=0.940, <0.960. Pick 0.942, which works, and is the system version on Ubuntu 22.04. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Currently, many test cases are not executed. A follow-up pull request will take care of that. In the meantime, continue allowing partial test coverage. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm · 2024-10-09T12:16:36Z

I have rewritten the history a little so that there is now a single commit that removes the files moved to the framework, obtained with David's script. I've also updated the framework submodule to the latest version.

eleuzi01

LGTM, thanks!

mpg

Re-approving after force-push - content is identical to what I previously approved, with the exception of the framework pointer updated to the latest version of Mbed-TLS/mbedtls-framework#55

…_analysis.py Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm · 2024-10-10T08:02:30Z

I just amended the last commit to switch the framework to the result of merging the companion PR.

mpg

LGTM after updating the framework pointer.

eleuzi01

LGTM but the CI is unhappy

gilles-peskine-arm · 2024-10-10T10:10:14Z

The CI failure looks infrastructure-related:

[2024-10-10T09:00:11.908Z] armclang: error: Failed to check out a license.
[2024-10-10T09:00:11.908Z] Unable to connect to the license server. Check that ARMLMD_LICENSE_FILE is set correctly, and the license server is available.

This might just be a network glitch, I'll try re-running the job.

gilles-peskine-arm added 9 commits October 3, 2024 17:23

Create a module to split test case collection from checks

1e5dbd3

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Split test case collection from checks

c8c83d0

Move the test case collection code out of check_test_cases.py and into its own module. This allows outcome analysis to depend only on the new module and not on check_test_cases.py. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Create a module to split branch-independent code out of analyze_outco…

9f930e0

…mes.py Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Missing NotImplementedError in abstract method

9d78e87

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Don't reuse a variable name inside a function

ad02d44

Use different names for task name, a task class and a task instance. The interpreter doesn't care, but it's less confusing for both humans and type checkers. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Pass KNOWN_TASKS as an argument to main

e41cde5

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm added needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) component-test Test framework and CI scripts priority-high High priority - will be reviewed soon labels Oct 3, 2024

gilles-peskine-arm mentioned this pull request Oct 3, 2024

Backport 3.6: Split check_test_cases.py and outcome_analysis.py #9669

Merged

5 tasks

gilles-peskine-arm force-pushed the analyze_outcome-split-dev branch from d85c6ce to da961e7 Compare October 4, 2024 09:34

gilles-peskine-arm mentioned this pull request Oct 4, 2024

Framework: Split check_test_cases.py and outcome_analysis.py Mbed-TLS/mbedtls-framework#55

Merged

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests labels Oct 4, 2024

This was referenced Oct 4, 2024

Test cases not executed: switch to enforcement mode #9593

Merged

[DI] Move scripts used in the CI to the framework Mbed-TLS/mbedtls-framework#52

Open

gilles-peskine-arm force-pushed the analyze_outcome-split-dev branch from da961e7 to 5d2d872 Compare October 4, 2024 13:55

eleuzi01 previously approved these changes Oct 7, 2024

View reviewed changes

mpg previously approved these changes Oct 8, 2024

View reviewed changes

tests/scripts/analyze_outcomes.py Show resolved Hide resolved

scripts/ci.requirements.txt Show resolved Hide resolved

gilles-peskine-arm added 5 commits October 8, 2024 19:37

Adjust paths for impending moves to the framework

3146772

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Default to allowing partial test coverage

96db2cc

Currently, many test cases are not executed. A follow-up pull request will take care of that. In the meantime, continue allowing partial test coverage. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm dismissed mpg’s stale review via 5ee987d October 9, 2024 12:14

gilles-peskine-arm dismissed eleuzi01’s stale review via 5ee987d October 9, 2024 12:14

gilles-peskine-arm force-pushed the analyze_outcome-split-dev branch from 5d2d872 to 5ee987d Compare October 9, 2024 12:14

gilles-peskine-arm removed the needs-reviewer This PR needs someone to pick it up for review label Oct 9, 2024

gilles-peskine-arm requested review from mpg and eleuzi01 October 9, 2024 12:22

eleuzi01 previously approved these changes Oct 9, 2024

View reviewed changes

mpg previously approved these changes Oct 10, 2024

View reviewed changes

mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 10, 2024

Update framework to the branch with collect_test_cases.py and outcome…

8fa4964

…_analysis.py Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm dismissed stale reviews from mpg and eleuzi01 via 8fa4964 October 10, 2024 08:01

gilles-peskine-arm force-pushed the analyze_outcome-split-dev branch from 5ee987d to 8fa4964 Compare October 10, 2024 08:01

gilles-peskine-arm requested review from mpg and eleuzi01 October 10, 2024 08:01

mpg approved these changes Oct 10, 2024

View reviewed changes

eleuzi01 approved these changes Oct 10, 2024

View reviewed changes

gilles-peskine-arm enabled auto-merge October 10, 2024 12:23

gilles-peskine-arm added this pull request to the merge queue Oct 10, 2024

Merged via the queue into Mbed-TLS:development with commit 80352ac Oct 10, 2024
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split check_test_cases.py and outcome_analysis.py #9668

Split check_test_cases.py and outcome_analysis.py #9668

gilles-peskine-arm commented Oct 3, 2024 •

edited

Loading

eleuzi01 left a comment

mpg left a comment

gilles-peskine-arm commented Oct 9, 2024

eleuzi01 left a comment

mpg left a comment

gilles-peskine-arm commented Oct 10, 2024

mpg left a comment

eleuzi01 left a comment

gilles-peskine-arm commented Oct 10, 2024

Split check_test_cases.py and outcome_analysis.py #9668

Split check_test_cases.py and outcome_analysis.py #9668

Conversation

gilles-peskine-arm commented Oct 3, 2024 • edited Loading

PR checklist

eleuzi01 left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Oct 9, 2024

eleuzi01 left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Oct 10, 2024

mpg left a comment

Choose a reason for hiding this comment

eleuzi01 left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Oct 10, 2024

gilles-peskine-arm commented Oct 3, 2024 •

edited

Loading