Skip to content
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

tests/sys/psa_crypto*: Fix failing tests #20178

Merged
merged 2 commits into from
Dec 16, 2023

Conversation

Einhornhool
Copy link
Contributor

Contribution description

PSA Crypto tests fail because of multiple issuse:

  1. Elliptic curve software implementations need larger stacks than the default stack size, so I increased that to 4096 bytes
  2. Secure element tests fail when running them without SE's attached. Since currently there are no SEs connected for testing, this PR removes the test folder from the SE tests. This way they will still be built (to test build configurations), but not run anymore.

Testing procedure

Run psa_crypto_ecdsa and psa_crypto_eddsa without runtime errors.

Issues/PRs references

Alternative fix for #20152 and #20150

@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Dec 13, 2023
@benpicco benpicco requested a review from maribu December 13, 2023 13:24
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 13, 2023
@riot-ci
Copy link

riot-ci commented Dec 13, 2023

Murdock results

✔️ PASSED

359cacd tests/sys/psa_crypto_*: increase stack size for sw ecc tests

Success Failures Total Runtime
77 0 78 02m:52s

Artifacts

@maribu
Copy link
Member

maribu commented Dec 13, 2023

Would you mind to also revert commits 167a70c and 754bbe4? Those should no longer be needed.

@Einhornhool
Copy link
Contributor Author

Would you mind to also revert commits 167a70c and 754bbe4? Those should no longer be needed.

Sure, done!

@maribu
Copy link
Member

maribu commented Dec 13, 2023

The CI doesn't like your commit messages (too long to display on a VT100 from 1978).

Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for fixing!

@maribu
Copy link
Member

maribu commented Dec 14, 2023

❌ check-commits / check-commits (commit-msg) (pull_request)

This still needs to be addressed. The first line of the commit message cannot exceed 80 chars.

Please squash at will.

@maribu
Copy link
Member

maribu commented Dec 14, 2023

cannot exceed 80 chars.

Sorry, my bad. In fact, 72 chars. I assume this is to allow git log to indent the message by 8 chars and still be readable on a VT100.

@Einhornhool
Copy link
Contributor Author

The CI doesn't like your commit messages (too long to display on a VT100 from 1978).

I saw, if everything's fine I'm going to squash them anyways

@benpicco benpicco added this pull request to the merge queue Dec 14, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 14, 2023
@MrKevinWeiss MrKevinWeiss added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: no fast fail don't abort PR build after first error CI: full build disable CI build filter labels Dec 15, 2023
@MrKevinWeiss
Copy link
Contributor

Ugg not this again. It seems you have to chase the memory increases. I enabled run hardware tests but maybe we also need to do the hack in the .murdock file to test only this app.

@mguetschow
Copy link
Contributor

mguetschow commented Dec 15, 2023

Couldn't you use cd test/sys/psa_crypto_eddsa && ../../../dist/tools/insufficient_memory/create_makefile.ci.sh to create the respective file? Done that in the past for my PRs, takes some time but worked reliably.

Edit: there's actually also a Makefile target, so make -C test/sys/psa_crypto_eddsa generate-Makefile.ci would be the way to go.

@MrKevinWeiss
Copy link
Contributor

MrKevinWeiss commented Dec 15, 2023

Yup, we also want to run the hardware tests as well.

@maribu maribu added this pull request to the merge queue Dec 16, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 16, 2023
@maribu maribu force-pushed the pr/fix-psa-build-config-tests branch from 3458971 to 359cacd Compare December 16, 2023 21:08
@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: full build disable CI build filter CI: no fast fail don't abort PR build after first error labels Dec 16, 2023
@maribu maribu added this pull request to the merge queue Dec 16, 2023
Merged via the queue into RIOT-OS:master with commit 59573a8 Dec 16, 2023
30 checks passed
@MrKevinWeiss
Copy link
Contributor

Thanks @Einhornhool and @maribu for going pushing this forward!

@maribu
Copy link
Member

maribu commented Dec 18, 2023

Note that on the long term we should still find a better solution. E.g. one of those:

  • Let PSA not depend optionally on the secure module (+ implementation of the PSA glue) required
  • Pull in a software-only fallback if the optional features are not used

@MrKevinWeiss MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants