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

Report configuration settings in the outcome file #9172

Merged
merged 10 commits into from
Jul 3, 2024
Merged

Report configuration settings in the outcome file #9172

merged 10 commits into from
Jul 3, 2024

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented May 23, 2024 •
edited
Loading

Record compile-time configuration options (boolean options only) for a test run in the outcome file. This gives us a handy way to know what configuration settings each configuration name corresponds to in the outcome file.

Accomplishes #3417 since analyze_outcomes will now warn (soon err) about options that are not getting enabled in any tests. But before marking that as resolved, we should remove test cases that we don't intend to pass, of which I'm sure there are a few, and we should add test jobs for missing coverage. I intend to do that in one or more follow-up pull requests.

This pull request adds a couple of test cases for combinations of options. Listing all interesting combinations of options is out of scope here, I think that should happen organically when we realize that a combination is of immediate interest.

PR checklist

@gilles-peskine-arm gilles-peskine-arm added needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) component-test Test framework and CI scripts priority-high High priority - will be reviewed soon labels May 23, 2024
@gilles-peskine-arm gilles-peskine-arm force-pushed the test_suite_config-booleans branch 2 times, most recently from 263a3f3 to c10076e Compare May 23, 2024 17:37
@ronald-cron-arm ronald-cron-arm self-requested a review May 27, 2024 07:46
@gilles-peskine-arm gilles-peskine-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-ci Needs to pass CI tests labels May 27, 2024
This was referenced May 27, 2024
Open
Closed
ronald-cron-arm
ronald-cron-arm requested changes May 28, 2024
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

This looks quite good to me. I have a few comments, questions.

tests/Makefile Show resolved Hide resolved
tests/.gitignore Outdated Show resolved Hide resolved
tests/CMakeLists.txt Show resolved Hide resolved
tests/scripts/generate_config_tests.py Outdated Show resolved Hide resolved
tests/scripts/generate_config_tests.py Outdated Show resolved Hide resolved
tests/scripts/generate_config_tests.py Outdated Show resolved Hide resolved
This was referenced May 28, 2024
Open
Open
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request May 29, 2024
Open
ronald-cron-arm
ronald-cron-arm previously approved these changes May 29, 2024
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm 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.

LGTM, thanks. I've looked at the naming a bit more and I've proposed some changes from "option" to "setting". But I am approving the PR anyway. If you find those suggestions useful, great otherwise never mind.

tests/scripts/generate_config_tests.py Outdated Show resolved Hide resolved
tests/scripts/generate_config_tests.py Outdated Show resolved Hide resolved
tests/scripts/generate_config_tests.py Outdated Show resolved Hide resolved
tests/scripts/generate_config_tests.py Outdated Show resolved Hide resolved
tests/scripts/generate_config_tests.py Outdated Show resolved Hide resolved
@gilles-peskine-arm
Copy link
Contributor Author

Thanks for the suggestions. I've gone and fully changed “option” to “setting”, and moved to “dependency” for a list of optionally-negated settings.

ronald-cron-arm
ronald-cron-arm requested changes May 30, 2024
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

A few final comments following the rewording.

tests/scripts/generate_config_tests.py Outdated Show resolved Hide resolved
tests/scripts/generate_config_tests.py Outdated Show resolved Hide resolved
tests/scripts/generate_config_tests.py Outdated Show resolved Hide resolved
ronald-cron-arm
ronald-cron-arm previously approved these changes May 30, 2024
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-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

@paul-elliott-arm paul-elliott-arm self-assigned this Jun 6, 2024
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Jun 20, 2024
Merged
4 tasks
@gilles-peskine-arm
Copy link
Contributor Author

I have rebased on top of the latest split-related work, and updated the path to psa/crypto_config.h in the new script. This ready for re-review as well as the 3.6 and framework PR. The 2.28 backport will come shortly.

ronald-cron-arm
ronald-cron-arm previously approved these changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

I've checked the rebase against the merge of #9251, LGTM.

paul-elliott-arm
paul-elliott-arm previously approved these changes Jun 26, 2024
View reviewed changes
Copy link
Member

@paul-elliott-arm paul-elliott-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

@paul-elliott-arm paul-elliott-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 Jun 26, 2024
@gilles-peskine-arm gilles-peskine-arm added the needs-backports Backports are missing or are pending review and approval. label Jun 26, 2024
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Jun 26, 2024
Merged
3 tasks
@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed approved Design and code approved - may be waiting for CI or backports labels Jul 1, 2024
@ronald-cron-arm
Copy link
Contributor

ronald-cron-arm commented Jul 2, 2024 •
edited
Loading

Ah sorry, there is framework conflict now.

@gilles-peskine-arm
Copy link
Contributor Author

The framework PR is merged. The problem is in the parent repository, where development says the framework is moving from A to B and this PR says the framework is moving from A to C. The fact that B is an ancestor of C in the framework doesn't enter into consideration. I need to either rebase or merge this PR so that the git knows which framework version to end with.

@gilles-peskine-arm
Merge remote-tracking branch 'dev' into test_suite_config-booleans
1b22dd8 
Reconcile the framework submodule heads to the latest one.
@gilles-peskine-arm
Copy link
Contributor Author

The fact that B is an ancestor of C in the framework doesn't enter into consideration.

Actually, the command line git tool is smart enough to merge automatically in this case. But the git implementation on GitHub isn't. I just observed this experimentally and this blog post explains it.

So I've pushed a merge commit with development.

ronald-cron-arm
ronald-cron-arm approved these changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-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

paul-elliott-arm
paul-elliott-arm approved these changes Jul 3, 2024
View reviewed changes
Copy link
Member

@paul-elliott-arm paul-elliott-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

@paul-elliott-arm paul-elliott-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 3, 2024
@ronald-cron-arm ronald-cron-arm added this pull request to the merge queue Jul 3, 2024
Merged via the queue into Mbed-TLS:development with commit 5e3c529 Jul 3, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronald-cron-arm ronald-cron-arm ronald-cron-arm approved these changes

@paul-elliott-arm paul-elliott-arm paul-elliott-arm approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports component-test Test framework and CI scripts needs-backports Backports are missing or are pending review and approval. priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
Roadmap Board for Mbed TLS
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gilles-peskine-arm @ronald-cron-arm @paul-elliott-arm