Cleanups in psa_compliance.py#221
Open
gilles-peskine-arm wants to merge 12 commits intoMbed-TLS:mainfrom
Open
Conversation
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
If given a non-empty ref, check out that ref and apply patches. If given an empty ref, keep whatever is there in the psa-arch-tests directory, without doing a checkout or applying patches. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Use sets to track membership rather than lists. It's more natural. In test_compliance, don't overwrite the mutable list passed by the caller. It's rude. This commit fixes a bug whereby an expected failure would lead to code that raised a `ValueError`, with an exception handler that expected this condition to raise `KeyError`. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Normally there's nothing on stderr, but if there is something (e.g. an assertion failure from libc or a sanitizer), it will be read and printed in order with respect to normal output. This will make errors easier to debug. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
5 tasks
5 tasks
mpg
requested changes
Oct 6, 2025
Contributor
mpg
left a comment
There was a problem hiding this comment.
Looking pretty good to me, just one docstring issue (plus a question and an optional nit).
mpg
reviewed
Oct 6, 2025
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This is useful when testing different versions or different build options, or to point to an existing set of worktrees. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
| PSA_ARCH_TESTS_REPO = 'https://github.com/ARM-software/psa-arch-tests.git' | ||
|
|
||
| #pylint: disable=too-many-branches,too-many-statements,too-many-locals | ||
| #pylint: disable=too-many-arguments,too-many-branches,too-many-statements,too-many-locals |
Contributor
Author
There was a problem hiding this comment.
This function should arguably be split into smaller pieces. But I don't think it's that bad, and it wouldn't solve too-many-arguments which is the only one that became necessary because of my changes. So I don't plan to break it up.
Contributor
There was a problem hiding this comment.
Note: if/when we break it up, I think the body of the try os.chdir() ... finally block would be a good candidate for a separate function, if only to reduce the level of indentation.
bjwtaylor
approved these changes
Feb 4, 2026
bjwtaylor
left a comment
There was a problem hiding this comment.
Looks good, though will need a rebase.
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.
A collection of improvements in
psa_compliance.py:Merge order:
PR checklist