Add conditional dependency to tests #2231

RonEld · 2018-11-26T12:30:11Z

Description

Some tests can run only if some compile time definitions meet a certain condition. For example, 8192 bit rsa key tests should not run if MBEDTLS_MPI_MAX_SIZE is less than 1024.
This PR adds a way to check compile time definitions values, for determining
whether to skip tests.

Status

READY

Requires Backporting

This is an enhancemnet, but to hte test infrastructure, so we may consider backporting this

Additional comments

Tested on Linux and K64F

Todos

Tests
Documentation
Changelog updated
Backported

Steps to test or reproduce

Set MBEDTLS_MPI_MAX_SIZE to 512, and run test_suite_rsa. The test "RSA Check Public key #6 (N exactly 8192 bits)" should be skipped

Add a way to check compile time defionitions values, for determining whether to skip tests.

Patater · 2018-11-26T16:04:43Z

tests/suites/test_suite_rsa.data

@@ -339,6 +339,7 @@ RSA Check Public key #5 (N smaller than 128 bits)
 mbedtls_rsa_check_pubkey:16:"7edcba9876543210deadbeefcafe4321":16:"3":MBEDTLS_ERR_RSA_KEY_CHECK_FAILED

 RSA Check Public key #6 (N exactly 8192 bits)


Does this test pass when MBEDTLS_MPI_MAX_SIZE is 1024?

yes. Obviously, I can't test it on target, because with MBEDTLS_MPI_MAX_SIZE of 1024, we get stack overflow, but on PC, tests pass

Isnt it possible to simply increase the stack size to a suitable value if the test is really needed?

I don't think this is a reasonable solution. Since we would like to test as closely to real use case.
Nonetheless, we still want to run this test ( N exactly 8192 bits) only when MBEDTLS_MPI_MAX_SIZE is >= 1024

I don't think this is a reasonable solution. Since we would like to test as closely to real use case.

Why not? A higher value is not an "unrealistic" situation, its probably just uncommon in the embedded setting. In any case, I would expect at least some of our tests to require slightly higher stack size for one or another reason...

Ideally we should get some feedback from the tests if we increase the stack consumption of a function. But that's more complex than a pass/fail unless we define some hard targets. Using a realistic stack size gives us a pass/fail result about stack consumption, but also means that we have to exclude some tests. I'd rather include as many tests as possible and use a larger-than-usual stack size. If we want feedback about stack usage we should get it as a metric with some instrumentation to determine peak stack usage for each test case, not as pass/fail.

In any case, we should skip some tests in some cases, when users change the default configuration

Patater · 2018-11-26T16:07:10Z

tests/scripts/generate_test_code.py

@@ -731,18 +732,19 @@ def gen_dep_check(dep_id, dep):
        raise GeneratorInputError("Dependency Id should be a positive "
                                  "integer.")
    _not, dep = ('!', dep[1:]) if dep[0] == '!' else ('', dep)
+    _defined = '' if re.search(r'(<=?|>=?|==)', dep) else 'defined'


Have we tested with ==? Like, depends_on:MBEDTLS_MPI_MAX_SIZE==1024?

I tested the regular expressions in a python shell:

>>> pattern=r'(<=?|>=?|==)' >>> def pat_search(): ... if re.search(pattern, dep): ... return '' ... else: ... return 'defined' >> dep='ABC>=123' >>> print pat_search() >>> dep='ABC==123' >>> print pat_search() >>> dep='ABC>123' >>> print pat_search() >>> dep='ABC=123' >>> dep='ABC<=123' >>> print pat_search() >>> dep='ABC=123' >>> print pat_search() defined

gilles-peskine-arm · 2018-11-27T09:16:18Z

tests/scripts/generate_test_code.py

@@ -184,7 +184,7 @@
 END_CASE_REGEX = r'/\*\s*END_CASE\s*\*/'

 DEPENDENCY_REGEX = r'depends_on:(?P<dependencies>.*)'
-C_IDENTIFIER_REGEX = r'!?[a-z_][a-z0-9_]*$'
+C_IDENTIFIER_REGEX = r'!?[a-z_][a-z0-9_]*(([<>]=?|==)[0-9]*)?$'


Please keep C_IDENTIFIER_REGEX the way it was and define a new macro for an identifier with a numerical constraint.

OK, but it is used only in validate_dependency(), and there the numerical constraint is also sent to be validated

What I suggested is to remove validate_dependency. Let the C preprocessor handle the job.

It is always best to catch problems as early as possible. in this case, in the python script, and not during compile time.
If one has a typo causing an invalid dependency, I would expect it to be raised while generating the tests, and not during compile time, or worse, on runtime, because any character works

tests/scripts/generate_test_code.py

Seperate the REGEX into identifier, condition and value, into groups, to behandled differently.

RonEld · 2018-11-27T14:39:28Z

@gilles-peskine-arm I have modified according to your comment, although I think that in gen_dep_check, it complicates the code more than it was

gilles-peskine-arm

Approved. I question the restriction that guards to be of the form <identifier> <relation-operation> <numerical-value>: I think we should allow arbitrary guards and let the C preprocessor validate them. However such guards are what we need most urgently, so I'm ok with going with them for now.

Patater

LGTM

simonbutcher · 2018-12-13T12:31:11Z

CI passed with exception of FreeBSD timing test. That's as good as a pass.

simonbutcher · 2018-12-24T17:06:42Z

Can you backport this please @RonEld?

RonEld · 2019-01-02T08:47:37Z

@sbutcher-arm I have asked @dgreen-arm to integrate this PR into #2075 as this PR modifies a file that doesn't exist yet in mbedtls-2.7, and this PR modifies on script for the on target test.
In addition, the motivation for this PR, was to have the tests successfully run on targets, so I thin k the best way for this backport, is to integrate it as part of the On target Test backport, or at least after it is merged.

RonEld · 2019-01-02T09:09:30Z

I have backported it though to mbedtls-2.16, in #2322

simonbutcher · 2019-01-08T13:53:46Z

Okay, I understand. This is a feature that fixes an issue, dependent on another test feature that hasn't yet been back ported.

I think that shouldn't hold up merging.

Add conditional dependency to tests

d5062de

Add a way to check compile time defionitions values, for determining whether to skip tests.

RonEld added enhancement mbed TLS team needs-review Every commit must be reviewed by at least two team members, component-platform Portability layer and build scripts labels Nov 26, 2018

Patater reviewed Nov 26, 2018

View reviewed changes

gilles-peskine-arm requested changes Nov 27, 2018

View reviewed changes

Separate REGEX of MACRO to groups

b9b3813

Seperate the REGEX into identifier, condition and value, into groups, to behandled differently.

RonEld force-pushed the conditional_dependency_in_tests branch from 2ef7214 to b9b3813 Compare November 27, 2018 14:38

gilles-peskine-arm approved these changes Nov 28, 2018

View reviewed changes

RonEld mentioned this pull request Nov 29, 2018

Rsa related On Target Tests crash with stack overflow #2194

Closed

Patater approved these changes Dec 3, 2018

View reviewed changes

RonEld removed the needs-review Every commit must be reviewed by at least two team members, label Dec 3, 2018

simonbutcher added the approved Design and code approved - may be waiting for CI or backports label Dec 13, 2018

RonEld mentioned this pull request Dec 19, 2018

Backport 2.7: Enable on-target execution of Mbed TLS unit tests #2075

Closed

simonbutcher added the needs-backports Backports are missing or are pending review and approval. label Dec 24, 2018

RonEld mentioned this pull request Jan 2, 2019

Backport 2.16: Add conditional dependency to tests #2322

Merged

Patater removed the needs-backports Backports are missing or are pending review and approval. label Jan 22, 2019

simonbutcher merged commit b9b3813 into Mbed-TLS:development Jan 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add conditional dependency to tests #2231

Add conditional dependency to tests #2231

RonEld commented Nov 26, 2018

Patater Nov 26, 2018

RonEld Nov 26, 2018 •

edited

Loading

andresag01 Nov 26, 2018

RonEld Nov 27, 2018

andresag01 Nov 27, 2018

gilles-peskine-arm Nov 27, 2018

RonEld Nov 29, 2018

Patater Nov 26, 2018

RonEld Nov 26, 2018

gilles-peskine-arm Nov 27, 2018

RonEld Nov 27, 2018

gilles-peskine-arm Nov 27, 2018 •

edited

Loading

RonEld Nov 28, 2018

RonEld commented Nov 27, 2018

gilles-peskine-arm left a comment

Patater left a comment

simonbutcher commented Dec 13, 2018

simonbutcher commented Dec 24, 2018

RonEld commented Jan 2, 2019

RonEld commented Jan 2, 2019

simonbutcher commented Jan 8, 2019

		@@ -339,6 +339,7 @@ RSA Check Public key #5 (N smaller than 128 bits)
		mbedtls_rsa_check_pubkey:16:"7edcba9876543210deadbeefcafe4321":16:"3":MBEDTLS_ERR_RSA_KEY_CHECK_FAILED

		RSA Check Public key #6 (N exactly 8192 bits)

Add conditional dependency to tests #2231

Add conditional dependency to tests #2231

Conversation

RonEld commented Nov 26, 2018

Description

Status

Requires Backporting

Additional comments

Todos

Steps to test or reproduce

Choose a reason for hiding this comment

RonEld Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld commented Nov 27, 2018

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Patater left a comment

Choose a reason for hiding this comment

simonbutcher commented Dec 13, 2018

simonbutcher commented Dec 24, 2018

RonEld commented Jan 2, 2019

RonEld commented Jan 2, 2019

simonbutcher commented Jan 8, 2019

RonEld Nov 26, 2018 •

edited

Loading

gilles-peskine-arm Nov 27, 2018 •

edited

Loading