-
Notifications
You must be signed in to change notification settings - Fork 38
Cleanups in psa_compliance.py #221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Cleanups in psa_compliance.py #221
Conversation
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
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 <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
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 <[email protected]>
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 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good to me, just one docstring issue (plus a question and an optional nit).
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
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 <[email protected]>
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
A collection of improvements in
psa_compliance.py
:Merge order:
PR checklist