-
Notifications
You must be signed in to change notification settings - Fork 96
Invasive testing strategy #251
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.
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
Invasive testing strategy #251
Conversation
|
|
||
| Solution: TODO. | ||
|
|
||
| See the [secure element driver interface test strategy](driver-interface-test-strategy.html) for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: currently in #246. Rendered HTML.
|
I read the document and it looks very good. I'd just like to check if I understood something right on a concrete example. Sometimes we have internal functions that we'd like to unit-test because that's the easiest and most efficient way. For example, That's mostly satisfying except that making the functions static would allow more compiler optimizations and possibly reduce the code size. This is a serious concern, as in another instance in the baremetal branch we observed significant reductions in code size (several hundred bytes) just by making a bunch of functions static (see https://github.com/ARMmbed/mbedtls-restricted/pull/652 for details). IIUC, the proposed rules would encourage us to:
Is that correct? |
|
@mpg Thanks for the feedback. This is a use case that I hadn't thought of. I agree with your solution. I'll add a paragraph about this to the document. If a function isn't part of the public API, I encourage declaring it in a header in |
I think that's good practice, and I wish it was implemented in Mbed TLS as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few typos, but otherwise looks good.
| * Portability: tests should work on every platform. Skipping tests on certain platforms may hide errors that are only apparent on such platforms. | ||
| * Maintenability: tests should only enforce the documented behavior of the product, to avoid extra work when the product's internal or implementation-specific behavior changes. We should also not give the impression that whatever the tests check is guaranteed behavior of the product which cannot change in future versions. | ||
|
|
||
| Where those goals conflit, we should at least mitigate the goals that cannot be fulfilled, and document the architectural choices and their rationale. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: conflit -> conflict
| * Correctness: we want to test the actual product, not a modified version, since conclusions drawn from a test of a modified product may not apply to the real product. | ||
| * Effacement: the product should not include features that are solely present for test purposes, since these increase the attack surface and the code size. | ||
| * Portability: tests should work on every platform. Skipping tests on certain platforms may hide errors that are only apparent on such platforms. | ||
| * Maintenability: tests should only enforce the documented behavior of the product, to avoid extra work when the product's internal or implementation-specific behavior changes. We should also not give the impression that whatever the tests check is guaranteed behavior of the product which cannot change in future versions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: Maintenability -> Maintainability
Also appears in a few other places.
Evaluate possible approaches for invasive testing. State some rules.
Clarify that using a header in library/ rather than include/ for internal functions is a rule, not just a possibility. As suggested by Manuel, state a rule for functions that need to be static for best optimization but that we want to unit-test.
When this option is enabled, the product includes additional interfaces that enable additional tests. This option should not be enabled in production, but is included in the "full" build to enable the extra tests.
Define MBEDTLS_STATIC_TESTABLE as described in mbed-crypto-invasive-testing.md. Since this is for internal library use only, define it in a header in library/. Since there is no suitable header, create one.
f6feccc to
e649e2e
Compare
|
I rebased on top of |
|
Moved to Mbed-TLS/mbedtls#3121 |
Documentation only. Explore potential strategies for invasive testing. Formulate some rules and guideline. Succinctly describe some solutions.
This is a proposal submitted for discussion.
I deliberately included solutions with “TODO” in them for things that we should do, but aren't doing yet.
I've changed headers to define macros mentioned explicitly in the strategy document so that one can start implementing parts of the strategy based on this PR. But implementing the proposed strategy is out of scope of this PR.