Skip to content

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

Merged
maribu merged 2 commits intoRIOT-OS:masterfrom
Einhornhool:pr/fix-psa-build-config-tests
Dec 16, 2023
Merged

tests/sys/psa_crypto*: Fix failing tests#20178
maribu merged 2 commits intoRIOT-OS:masterfrom
Einhornhool:pr/fix-psa-build-config-tests

Conversation

@Einhornhool
Copy link
Copy Markdown
Contributor

@Einhornhool Einhornhool commented Dec 13, 2023

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
Copy Markdown

riot-ci commented Dec 13, 2023 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Contributor Author

Einhornhool commented Dec 13, 2023

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

Sure, done!

@maribu
Copy link
Copy Markdown
Member

maribu commented Dec 13, 2023

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

mguetschow
mguetschow approved these changes Dec 14, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

Einhornhool commented Dec 14, 2023

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

@Einhornhool Einhornhool force-pushed the pr/fix-psa-build-config-tests branch from d35e88c to 3458971 Compare December 14, 2023 12:14
@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
Copy Markdown
Contributor

MrKevinWeiss commented Dec 15, 2023

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
Copy Markdown
Contributor

mguetschow commented Dec 15, 2023 •
edited
Loading

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
Copy Markdown
Contributor

MrKevinWeiss commented Dec 15, 2023 •
edited
Loading

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 enabled auto-merge 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
@OlegHahm OlegHahm mentioned this pull request Dec 17, 2023
Merged
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Dec 18, 2023

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

@maribu
Copy link
Copy Markdown
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

Reviewers

@maribu maribu maribu approved these changes

@mguetschow mguetschow mguetschow approved these changes

@smlng smlng Awaiting requested review from smlng

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

Assignees

No one assigned

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

Milestone

Release 2024.01

Development

Successfully merging this pull request may close these issues.

6 participants

@Einhornhool @riot-ci @maribu @MrKevinWeiss @mguetschow @benpicco