Skip to content

Conversation

@bjwtaylor
Copy link

@bjwtaylor bjwtaylor commented Oct 14, 2025

Description

Add adapted lcov.sh to the framework. I've uploaded the rudimentary test script that I used to validate the change here https://github.com/bjwtaylor/mbedtls/blob/lcov-test/scripts/test.sh. It doesn't inspect the report, however it validates it has been created not sure if this is useful in this PR? Let me know what people think? contributes #93

This PR is part of a multistage PR, however the code changes involved appear to not effect the CI so it can be merged in any order. However it is suggested it is merged in the following order.

  1. Add adapted lcov.sh to the framework #224
  2. Update lcov.sh paths in make files TF-PSA-Crypto#536
  3. Move lcov 3.6 mbedtls#10452
  4. Remove lcov.sh as this will be moved to the framework mbedtls#10450

PR checklist

@bjwtaylor bjwtaylor 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 size-s Estimated task size: small (~2d) priority-medium Medium priority - this can be reviewed as time permits labels Oct 15, 2025
@valeriosetti valeriosetti self-requested a review October 16, 2025 11:37
@valeriosetti
Copy link
Contributor

What I suggest when moving files for the next time:

  • 1 commit to move the files
  • 1 or more commit to change the files

In this way it's easier to tell what was done do adapt the script to work with mbedtls 4.0/3.6 and tf-psa-crypto. I can still do it locally of course, but it's less immediate.

@bjwtaylor bjwtaylor marked this pull request as ready for review October 16, 2025 11:56
@valeriosetti
Copy link
Contributor

I've uploaded the rudimentary test script that I used to validate the change here https://github.com/bjwtaylor/mbedtls/blob/lcov-test/scripts/test.sh.

This is just my curiosity, not a problem for this PR: does it make any difference in terms of coverage if we're building in-tree or out-of-tree?

@bjwtaylor bjwtaylor added needs-work and removed 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 labels Oct 17, 2025
@bjwtaylor
Copy link
Author

I've uploaded the rudimentary test script that I used to validate the change here https://github.com/bjwtaylor/mbedtls/blob/lcov-test/scripts/test.sh.

This is just my curiosity, not a problem for this PR: does it make any difference in terms of coverage if we're building in-tree or out-of-tree?

So, it shouldn't do. The coverage is based on the tests you run, so if you are running the same tests you should get the same result. It would be an interesting test to do though.

Ben Taylor added 2 commits October 21, 2025 11:21
@mpg mpg added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Oct 24, 2025
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!

@@ -0,0 +1,101 @@
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: not a big fan of the commit structure here. In the future, please start with an identical copy, and make changes as a separate commit. This makes it much easier to see what changed.

As a general rule, moves (of files, functions, or large chunks of code), as well a large whitespace changes (re-indent / fix code style), should be separated as much as possible from actual changes. This makes it much easier to review what's going on.

(No need to rewrite history here at this point, but please keep in mind for the future.)

@mpg mpg 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 Oct 24, 2025
@mpg mpg merged commit 875ec30 into Mbed-TLS:main Oct 24, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Design and code approved - may be waiting for CI or backports priority-medium Medium priority - this can be reviewed as time permits size-s Estimated task size: small (~2d)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants