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.

Sign up for GitHub

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

Jump to bottom

Move crypto test suites #9394

Merged
merged 34 commits into from
Jul 24, 2024
Merged

Move crypto test suites #9394

merged 34 commits into from
Jul 24, 2024

Conversation

ronald-cron-arm
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm commented Jul 15, 2024 •
edited by davidhorstmann-arm
Loading

Description

Repo split preparation, move crypto test suites to tf-psa-crypto.
Companion mbedtls-framework PR: Mbed-TLS/mbedtls-framework#36
Fix #9266

PR checklist

@ronald-cron-arm ronald-cron-arm added enhancement component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon labels Jul 15, 2024
@ronald-cron-arm ronald-cron-arm force-pushed the move-crypto-test-suites branch 2 times, most recently from a67a3fc to 1e41203 Compare July 16, 2024 06:28
@ronald-cron-arm ronald-cron-arm force-pushed the move-crypto-test-suites branch from 1e41203 to 20ca55d Compare July 16, 2024 12:47
@bensze01 bensze01 mentioned this pull request Jul 17, 2024
Merged
@ronald-cron-arm ronald-cron-arm force-pushed the move-crypto-test-suites branch 4 times, most recently from 9f9a945 to b18a364 Compare July 18, 2024 07:59
@ronald-cron-arm ronald-cron-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-work needs-ci Needs to pass CI tests labels Jul 18, 2024
@minosgalanakis minosgalanakis self-requested a review July 18, 2024 14:59
@minosgalanakis minosgalanakis removed the needs-reviewer This PR needs someone to pick it up for review label Jul 18, 2024
minosgalanakis
minosgalanakis reviewed Jul 18, 2024
View reviewed changes
tf-psa-crypto/tests/CMakeLists.txt
@@ -0,0 +1,362 @@
set(libs
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is duplicating {MBEDTLS-ROOT}/tests/CMakeLists.txt. It is effectively a large ammount of duplicated code. I am assuming those files are expected to diverge later on, so could we possibly source one from the other when they need to be identical?

Copy link
Contributor Author

@ronald-cron-arm ronald-cron-arm Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Eventually there will be one in mbedtls and one in TF-PSA-Crypto with quite similar content and we do not plan to share anything there. Otherwise as of this PR, the two files diverge in the two following commits. I've done it that way to highlight in the commit history that the new tf-psa-crypto/tests/CMakeLists.txt file is just a derivation of the one we have in Mbed TLS and you can see the derivation in the commit "Add tf-psa-crypto test suites build".

This was referenced Jul 23, 2024
Closed
Merged
minosgalanakis
minosgalanakis previously approved these changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

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

LGTM

davidhorstmann-arm
davidhorstmann-arm previously approved these changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm 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!

davidhorstmann-arm
davidhorstmann-arm previously approved these changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm 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!

@ronald-cron-arm ronald-cron-arm force-pushed the move-crypto-test-suites branch from ba045b2 to b16b8c2 Compare July 23, 2024 16:20
@ronald-cron-arm
Copy link
Contributor Author

@davidhorstmann-arm @minosgalanakis last version: updated the framework to the merge of #38 instead of #36 and updated the PSA test wrappers.

minosgalanakis
minosgalanakis approved these changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

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

LGTM

davidhorstmann-arm
davidhorstmann-arm approved these changes Jul 24, 2024
View reviewed changes
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm 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!

@davidhorstmann-arm davidhorstmann-arm 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 Jul 24, 2024
@ronald-cron-arm ronald-cron-arm added this pull request to the merge queue Jul 24, 2024
@minosgalanakis minosgalanakis mentioned this pull request Jul 24, 2024
Closed
4 tasks
Merged via the queue into Mbed-TLS:development with commit f938f4f Jul 24, 2024
6 checks passed
@minosgalanakis minosgalanakis mentioned this pull request Jul 24, 2024
Merged
5 tasks
@ronald-cron-arm ronald-cron-arm mentioned this pull request Jul 29, 2024
Merged
5 tasks
@ronald-cron-arm ronald-cron-arm mentioned this pull request Aug 30, 2024
Merged
5 tasks
gilles-peskine-arm
gilles-peskine-arm reviewed Sep 11, 2024
View reviewed changes
tests/Makefile
perl scripts/run-test-suites.pl $(TEST_FLAGS) --skip=$(SKIP_TEST_SUITES)
cd ../tf-psa-crypto/tests && perl ../../tests/scripts/run-test-suites.pl $(TEST_FLAGS) --skip=$(SKIP_TEST_SUITES)
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: the crypto tests are skipped if a TLS test fails.

Why doesn't run-test-suites.pl take care of looking for the subproject's test suites? (Like ctest does.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't run-test-suites.pl take care of looking for the subproject's test suites?

#9558

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm left review comments

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm left review comments

@minosgalanakis minosgalanakis minosgalanakis approved these changes

@davidhorstmann-arm davidhorstmann-arm davidhorstmann-arm approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports component-crypto Crypto primitives and low-level interfaces enhancement priority-high High priority - will be reviewed soon
Projects
No open projects
Roadmap Board for Mbed TLS
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move Mbed TLS crypto test suites to tf-psa-crypto directory
5 participants
@ronald-cron-arm @gilles-peskine-arm @tom-cosgrove-arm @minosgalanakis @davidhorstmann-arm