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 test helpers from Mbed TLS #65

Merged
merged 11 commits into from
Nov 18, 2024
Merged

Move test helpers from Mbed TLS #65

merged 11 commits into from
Nov 18, 2024

Conversation

davidhorstmann-arm
Copy link
Contributor

Companion PR to Mbed-TLS/mbedtls#9756. Move test helpers to the framework from Mbed TLS.

@davidhorstmann-arm davidhorstmann-arm force-pushed the dev/davidhorstmann-arm/add-some-test-helpers branch 2 times, most recently from 04800b8 to 52aecf8 Compare November 8, 2024 11:58
@davidhorstmann-arm davidhorstmann-arm mentioned this pull request Nov 8, 2024
Merged
5 tasks
@mpg mpg mentioned this pull request Nov 12, 2024
Merged
5 tasks
mpg and others added 3 commits November 12, 2024 11:36
@mpg
Move all-{core,helpers}.sh out of Mbed TLS
06f8f9e 
Signed-off-by: Manuel Pégourié-Gonnard <mpg@elzevir.fr>
@mpg
Update framework pointer
8f00e5e 
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg
Update path to all-core.sh
30e8eb9 
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg mpg self-requested a review November 13, 2024 10:57
eleuzi01
eleuzi01 previously approved these changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

@eleuzi01 eleuzi01 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 dismissed eleuzi01’s stale review November 14, 2024 08:12

The merge-base changed after approval.

mpg and others added 6 commits November 14, 2024 09:14
@mpg
Update framework pointer
845cc54 
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg
Merge pull request #9769 from mpg/move-all-core-dev
824a8dc 
Move all-core and all-helpers to the framework
@davidhorstmann-arm
Move test helpers into the framework
dfd518f 
Move everything in tests/src and tests/include except for TLS-only
things, the alt-dummy headers and the generated PSA test wrappers.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
@davidhorstmann-arm
Merge branch 'tmp-branch-move-files-to-framework' into dev/davidhorst…
af90340 
…mann-arm/add-some-test-helpers
@davidhorstmann-arm
Update default path for test_keys.h
41cca8d 
This has now moved to the framework.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
@davidhorstmann-arm
Always include psa/crypto.h
a2b59be 
Previously this was not included when in 3.6 and PSA_CRYPTO_CLIENT was
defined.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
@davidhorstmann-arm davidhorstmann-arm force-pushed the dev/davidhorstmann-arm/add-some-test-helpers branch from c70752c to a2b59be Compare November 14, 2024 12:27
@davidhorstmann-arm
Update path in all-core.sh
defe386 
Signed-off-by: David Horstmann <david.horstmann@arm.com>
mpg
mpg reviewed Nov 15, 2024
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me on code inspection, but the CI isn't happy.

I'll note that some of the files moved were not identical between 3.6 and development, and when that's the case the version that landed here is the version from development. The differences are:

  1. In tests/include/test/psa_crypto_helpers.h a guard was MBEDTLS_PSA_CRYPTO_C in 3.6 and MBEDTLS_PSA_CRYPTO_CLIENT in development (and now framework) - I think keeping CLIENT for both is OK.
  2. In tests/src/helpers.c development (and new framework) had a declaration for mbedtls_test_hook_error_add which 3.6 doesn't. In 3.6 this declaration lives in scripts/data_files/error.fmt. In development it was recently moved by Split error.h and move back error.c to mbedtls mbedtls#9693. I expect the 3.6 PR will resolve that (perhaps by backporting the relevant parts from Split error.h and move back error.c to mbedtls mbedtls#9693).

@davidhorstmann-arm
Add missing include path in libtestdriver helper
45b567d 
Signed-off-by: David Horstmann <david.horstmann@arm.com>
@davidhorstmann-arm davidhorstmann-arm added needs-review Every commit must be reviewed by at least two team members, needs-work needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon and removed needs-review Every commit must be reviewed by at least two team members, needs-work labels Nov 15, 2024
mpg
mpg approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@mpg mpg 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!

@mpg
Copy link
Contributor

mpg commented Nov 18, 2024

CI is green on #9756 and #9766 so I'm removing "needs-ci".

@mpg mpg added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests labels Nov 18, 2024
@mpg mpg requested a review from eleuzi01 November 18, 2024 11:44
@eleuzi01 eleuzi01 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 Nov 18, 2024
@mpg mpg merged commit 862fde2 into main Nov 18, 2024
1 of 2 checks passed
@gilles-peskine-arm
Copy link
Contributor

In tests/src/helpers.c development (and new framework) had a declaration for mbedtls_test_hook_error_add which 3.6 doesn't. In 3.6 this declaration lives in scripts/data_files/error.fmt. In development it was recently moved by Mbed-TLS/mbedtls#9693. I expect the 3.6 PR will resolve that (perhaps by backporting the relevant parts from Mbed-TLS/mbedtls#9693).

I hope to remove MBEDTLS_ERROR_ADD from Mbed TLS 4.0 (part of Mbed-TLS/mbedtls#8501 which hopefully we'll do in Q1 2025). So preferably everything related to it should stay in the 3.6 branch, and not pollute the framework.

@davidhorstmann-arm
Copy link
Contributor Author

So preferably everything related to it should stay in the 3.6 branch, and not pollute the framework.

We haven't moved anything from the 3.6 branch into the framework, we've just removed a definition from 3.6. It currently exists in 4.x and will be moved to the framework here (removing it should be easy enough).

@davidhorstmann-arm davidhorstmann-arm mentioned this pull request Nov 20, 2024
Closed
@davidhorstmann-arm davidhorstmann-arm mentioned this pull request Dec 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eleuzi01 eleuzi01 eleuzi01 approved these changes

@mpg mpg mpg approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports priority-high High priority - will be reviewed soon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@davidhorstmann-arm @mpg @gilles-peskine-arm @eleuzi01