Add bignum test case generation script #6093

wernerlewis · 2022-07-18T17:03:18Z

Description

Resolves #5921

Adds a test case generation framework for bignum tests, similar to generate_psa_tests.py. This PR adds a few bignum arithmetic test cases to show the usage of the test framework, and it will be expanded to cover more bignum/ECC functionality in subsequent PRs.

This PR should ideally be merged after #6070. The temporary radix argument commit can then be removed before merging this PR.

Status

READY

Requires Backporting

Yes 2.28 #6307

Adds python script for generation of bignum test cases, with initial classes for mpi_cmp_mpi test cases. Build scripts are updated to generate test data. Signed-off-by: Werner Lewis <[email protected]>

Signed-off-by: Werner Lewis <[email protected]>

minosgalanakis

Overall looks good, and will do the job it is supposed to do. My main concerns are:

Complexity. The code may be hard to maintain, especially since we are combining iterator and class generation. It assumes the reviewer is quite apt with modern Python.
Error handling. A lot of edge cases rely on assumed correct input, especially in earlier stages of inheritance.

Would having a document or a comment block descibing how you can easily add new tests be benefical?

tests/scripts/generate_bignum_tests.py

minosgalanakis · 2022-08-22T13:12:55Z

tests/scripts/generate_bignum_tests.py

+
+    def write_test_data_file(self, basename: str,
+                             test_cases: Iterable[test_case.TestCase]) -> None:
+        """Write the test cases to a .data file.


I assume the formatting is to comply with PEP257? We do not follow strict PEP8 guidelines but it is nice to see a standard approach.

tests/scripts/generate_bignum_tests.py

tests/CMakeLists.txt

tests/scripts/generate_bignum_tests.py

gilles-peskine-arm · 2022-08-22T12:16:27Z

tests/scripts/generate_bignum_tests.py

+
+    @property
+    def description(self) -> str:
+        """Create a numbered test description."""


Numbered test case descriptions should be a last resort, when there are multiple test cases with pseudorandom data. We should write/generate meaningful test case descriptions as much as possible.

Looking at the output, I think you intended for the numbering to be additional to meaningful descriptions? That's well-meaning, but it does have a downside, which is that the numbering will change whenever we add test cases, or change the order in which they're generated. This would make it hard to compare test results across different commits. So please don't systematically add numbering to test case names.

I think that having numbered test is useful and is worth the cost of that 3-4 characters in the title. Comparing test results across different commits is only a problem if we added test cases or changed generation order between the two commits. Also, in the overwhelming majority of cases we are looking at the tests within a single commit, where having a number makes identifying the failing test cases much easier.

Could we please keep the test case numbering?

There is value in having an easy-to-compare unique identifier on test cases. That unique identifier doesn't need to be meaningful, so it can be a number. Now the question is, what kind of stability do we expect on this number?

At one extreme, we can just use the line number. This changes pretty much whenever we add or remove test cases (or comments). Nice properties: lookup by number is extremely easy; the numbers are increasing (but not consecutive); the numbering can be done fully automatically by the test framework.

At another extreme, we can assign a number (or other short identifier) when we write a test case, and change to it. Advantage: the unique identifier is permanent. Downside: has to be managed manually, may cause conflicts if two concurrent tracks of work add the same unique identifier to different test cases.

With the intermediate solution we'd be introducing here, test case numbers would apply only to automatically generated test cases, so this isn't a general solution. We can easily tweak the scope of the numbering (per test function, per data file…) but any scope has its own downside (per test function: makes it hard to figure out what the unique part is; per test suite: makes the numbers unstable when we add unrelated test cases). I think this is not a good design for test case numbering. So I maintain my opposition.

I think the idea of test numbering is a good thing. I, too, have spent time copy-pasting descriptions around to search them, and sometimes looking at the wrong test case because I missed a difference in the description. So far it's been low on my tech debt list, but seeing I'm not the only one interested, then by all means let's add it to our tech debt. But let's first agree on a design, and introduce it as its own feature rather than part of some unrelated work.

I am happy to have discussion about the design, but could we please not block this PR with that discussion?

This is a test framework, whatever we decide, won't affect the test cases we add while discussing the matter. I really would like to start using this in the Bignum refactoring work as soon as possible. We could tackle the global numbering problem in a different PR.

could we please not block this PR with that discussion?

Agreed, absolutely. Test case numbers are going to cause merge conflicts if we work on this script in parallel, which is likely in the near future. So let's not put them in now, and add them later if we decide on that strategy.

Great, thanks, sounds good!

Regarding the temporary presence of test numbering in automated tests:

The script output is generated files, they won't be committed and won't cause merge conflicts. If we work in parallel on the script, we won't change the part that adds the numbering and the other PR will likely remove it. Again, it seems as a clean merge, I can't see the conflict.

On the Bignum refactoring side, while we don't have the global numbering solution, having this local numbering would be really helpful. Some tests have hundreds of test cases, it is very difficult to navigate them without any numbering.

The script output is generated files, they won't be committed and won't cause merge conflicts

Well, I was thinking of 2.2x, where they are committed. But it's true that on 2.2x, we typically have quick cycle from making the backport to merging it, so the risk of conflicts is small.

Is this going to be backported? I know it says "Yes" in the description, but this is clearly a new feature. And many of the newly-generated tests won't be applicable to bignum-2.28. Of course, having them the same has advantages, but I thought we'd agreed that bignum-development and bignum-2.28 would diverge?

This is a test framework, which is helpful to have on both and the test framework doesn't have to (or shouldn't) diverge. Also, this would make backporting tests easier. (We are adding more tests to other areas as well, not just the new code.)

tests/scripts/generate_bignum_tests.py

Signed-off-by: Werner Lewis <[email protected]>

minosgalanakis

Few nitpicking items and design discussions. They do not need refactor, but would be good to have it other reviewers request changes.

tests/scripts/generate_bignum_tests.py

minosgalanakis · 2022-09-13T09:57:22Z

tests/scripts/generate_bignum_tests.py

+        super().__init__(val_a.strip("-"), val_b.strip("-"))
+
+
+class BignumAdd(BignumOperation):


Not a blocker.

To my understanding tests will be implemented by implementing BignumOperation and overriding the result(). Depending on how much that functionality is about to grow, it may be of value to move those classes onto a separate files.

There's a strong unifying theme here, so I don't see any value in splitting. We may want to reorganize some things when we start generating ECC arithmetic tests, by creating a common module used by both bignum and ecc test generators, but I think it's too early to tell what code we'll want to share.

tests/scripts/generate_bignum_tests.py

scripts/mbedtls_dev/test_generation.py

minosgalanakis · 2022-09-13T10:45:37Z

scripts/mbedtls_dev/test_generation.py

+    targets = {} # type: Dict[str, Callable[..., Iterable[test_case.TestCase]]]
+
+    def generate_target(self, name: str, *target_args) -> None:
+        """Generate cases and write to data file for a target.


Inconsistent whitepace after function describing docstring

minosgalanakis · 2022-09-13T10:55:29Z

scripts/mbedtls_dev/test_generation.py

+    test_name = ""
+
+    def __new__(cls, *args, **kwargs):
+        # pylint: disable=unused-argument


Since we are moving to metaclasses, we could consider doing something more fancy than incrementing a counter.

We have two classes BaseTarget and TestGenerator which are mostly consumed by different modules, but in effect are doing the same thing. The could be merged into one class that works for everything, and optimised in consecutive pr's without having to change the consuming interfaces.

Methods like:

TestGenerator

write_test_data_file

are generic IO methods which can be common for both and BaseTarget.generate_target() TestGenerator.generate_tests() can be unified, or simply renamed.

Or for a more lazy approach we could even have a self.subtype == "PSA_Test_Target/ BigNum_Test_Target"

I don't understand why we'd want anything fancier here.

Also please keep in mind that the average maintainer of this file is primarily a C programmer, and not a Python expert.

This was more of a proposal as a stepping stone if we are aiming to intergrate those two into the future. Not a strong ask by any means ;)

tests/scripts/generate_bignum_tests.py

gilles-peskine-arm

A couple of minor documentation issues, and an unnecessary cast. Other than that looks good to me.

tests/scripts/generate_bignum_tests.py

scripts/mbedtls_dev/test_generation.py

gilles-peskine-arm · 2022-09-13T18:44:41Z

scripts/mbedtls_dev/test_generation.py

+    test_name = ""
+
+    def __new__(cls, *args, **kwargs):
+        # pylint: disable=unused-argument


I don't understand why we'd want anything fancier here.

Also please keep in mind that the average maintainer of this file is primarily a C programmer, and not a Python expert.

tests/scripts/generate_bignum_tests.py

gilles-peskine-arm · 2022-09-13T18:50:24Z

tests/scripts/generate_bignum_tests.py

+        super().__init__(val_a.strip("-"), val_b.strip("-"))
+
+
+class BignumAdd(BignumOperation):


There's a strong unifying theme here, so I don't see any value in splitting. We may want to reorganize some things when we start generating ECC arithmetic tests, by creating a common module used by both bignum and ecc test generators, but I think it's too early to tell what code we'll want to share.

Signed-off-by: Werner Lewis <[email protected]>

This reverts commit f156c43. Adds a comment to explain reasoning for current implementation. Signed-off-by: Werner Lewis <[email protected]>

Signed-off-by: Werner Lewis <[email protected]>

Wrapper function for itertools.combinations_with_replacement, with explicit cast due to imprecise typing with older versions of mypy. Signed-off-by: Werner Lewis <[email protected]>

Signed-off-by: Werner Lewis <[email protected]>

Previous changes used the docstring of the test_generation module, which does not inform a user about the script. Signed-off-by: Werner Lewis <[email protected]>

minosgalanakis · 2022-09-16T16:59:52Z

tests/scripts/generate_bignum_tests.py

+
+if __name__ == '__main__':
+    # Use the section of the docstring relevant to the CLI as description
+    test_generation.main(sys.argv[1:], "\n".join(__doc__.splitlines()[:4]))


This is precicely why using "doc" was established. The same can be achieved by

__short_doc __ = """Generate test data for bignum functions. "" __file_help__ = """With no arguments, generate all test data. With non-option arguments, __doc__ = __short_doc __ + __file_help__ generate only the specified files. Class structure: Child classes of test_generation.BaseTarget (file targets) represent an output """ ..... ..... ..... test_generation.main(sys.argv[1:], __file_help__ )) if __name__ == "__main__": print(__doc__)

This shall not block this PR, this is just for the purposes of discussion

minosgalanakis

Looks good to me.

gilles-peskine-arm

Approved. I have two comments but neither are blockers.

In the interest of making it possible to use this script in the ongoing bignum work as soon as possible, I intend to merge this as soon as CI passes. We still need to make a 2.28 backport (which should be an identical script, plus in 2.28 we commit the output into version control).

gilles-peskine-arm · 2022-09-16T19:24:30Z

tests/scripts/generate_bignum_tests.py

+    The return value is cast, as older versions of mypy are unable to derive
+    the specific type returned by itertools.combinations_with_replacement.


Non-blocker: this information is relevant when reading the function's code, but not when using the function. So it should be a comment, not part of the documentation string.

#6296 has fixes for this and the other non-blocker issues I raised in this review.

gilles-peskine-arm · 2022-09-16T19:32:25Z

scripts/mbedtls_dev/test_generation.py

+    # The `--directory` option is interpreted relative to the directory from
+    # which the script is invoked, but the default is relative to the root of
+    # the mbedtls tree. The default should not be set above, but instead after
+    # `build_tree.chdir_to_root()` is called.


Well, not quite. --directory is still interpreted relative to the root of the mbedtls tree. An abspath call is missing somewhere (or alternatively we could stop using chdir, but that would be annoying).

But since this is preexisting (I know it worked when I originally wrote it, but I might have broken it before I even committed), this is a non-blocker.

gilles-peskine-arm · 2022-09-16T19:39:05Z

scripts/mbedtls_dev/test_generation.py

@@ -0,0 +1,219 @@
+"""Common test generation classes and main function.


I only just realized, but we already have test code generation (tests/scripts/generate_test_code.py, turns .function files into .c), so “test generation” is ambiguous: we should specify that this is test data generation. I propose (in a follow-up) to rename this module to test_data_generation.

gilles-peskine-arm · 2022-09-16T19:53:37Z

scripts/mbedtls_dev/test_generation.py

+from mbedtls_dev import build_tree
+from mbedtls_dev import test_case


These should be relative imports. No reason to assume that scripts is in the search path.

Again, this is a preexisting bug which doesn't matter in our normal usage, so not a blocker.

wernerlewis added needs-preceding-pr Requires another PR to be merged first component-test Test framework and CI scripts labels Jul 18, 2022

wernerlewis force-pushed the bignum_test_script branch 5 times, most recently from c899567 to 1c3affd Compare July 20, 2022 09:34

wernerlewis added 9 commits August 8, 2022 11:58

Add bignum test generation framework

8b2df74

Adds python script for generation of bignum test cases, with initial classes for mpi_cmp_mpi test cases. Build scripts are updated to generate test data. Signed-off-by: Werner Lewis <[email protected]>

Add test generation for bignum cmp variant

69a92ce

Signed-off-by: Werner Lewis <[email protected]>

Add test case generation for bignum add

86caf85

Signed-off-by: Werner Lewis <[email protected]>

Sort tests when generating cases

a51fe2b

Signed-off-by: Werner Lewis <[email protected]>

Remove set() to preserve test case order

b17ca8a

Signed-off-by: Werner Lewis <[email protected]>

Fix type issues

c442f6a

Signed-off-by: Werner Lewis <[email protected]>

Remove is None from if statement

265e051

Signed-off-by: Werner Lewis <[email protected]>

Fix incorrect indentation

6a31396

Signed-off-by: Werner Lewis <[email protected]>

Fix CMake change failures on Windows

75ef944

Signed-off-by: Werner Lewis <[email protected]>

wernerlewis force-pushed the bignum_test_script branch from 74b4634 to 75ef944 Compare August 8, 2022 11:05

wernerlewis removed the needs-preceding-pr Requires another PR to be merged first label Aug 8, 2022

wernerlewis 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 labels Aug 8, 2022

gilles-peskine-arm self-requested a review August 11, 2022 18:43

gilles-peskine-arm mentioned this pull request Aug 15, 2022

Bignum: Montgomery multiplication from bignum prototype #6083

Merged

4 tasks

yanesca requested a review from minosgalanakis August 22, 2022 10:47

yanesca added the priority-high High priority - will be reviewed soon label Aug 22, 2022

minosgalanakis previously approved these changes Aug 22, 2022

View reviewed changes

gilles-peskine-arm requested changes Aug 22, 2022

View reviewed changes

wernerlewis added 2 commits September 2, 2022 12:57

Remove unused imports

5601308

Signed-off-by: Werner Lewis <[email protected]>

Use simpler int to hex string conversion

855e45c

Signed-off-by: Werner Lewis <[email protected]>

tom-cosgrove-arm mentioned this pull request Sep 12, 2022

Split bignum tests and re-do in python framework #6274

Closed

wernerlewis added 2 commits September 12, 2022 17:34

Move symbol definition out of __init__

1fade8a

Signed-off-by: Werner Lewis <[email protected]>

Replace L/R inputs with A/B

3dc4519

Signed-off-by: Werner Lewis <[email protected]>

minosgalanakis reviewed Sep 13, 2022

View reviewed changes

gilles-peskine-arm requested changes Sep 13, 2022

View reviewed changes

wernerlewis added 7 commits September 14, 2022 16:52

Update comments/docstrings in TestGenerator

34d6d3e

Signed-off-by: Werner Lewis <[email protected]>

Add toggle for test case count in descriptions

858cffd

Signed-off-by: Werner Lewis <[email protected]>

Remove argparser default for directory

00d0242

This reverts commit f156c43. Adds a comment to explain reasoning for current implementation. Signed-off-by: Werner Lewis <[email protected]>

Use typing.cast instead of unqualified cast

b6e8091

Signed-off-by: Werner Lewis <[email protected]>

Add combination_pairs helper function

ac446c8

Wrapper function for itertools.combinations_with_replacement, with explicit cast due to imprecise typing with older versions of mypy. Signed-off-by: Werner Lewis <[email protected]>

Update references to file targets in docstrings

52ae326

Signed-off-by: Werner Lewis <[email protected]>

Fix setting for default test suite directory

07c830c

Signed-off-by: Werner Lewis <[email protected]>

wernerlewis added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Sep 15, 2022

Use a script specific description in CLI help

c2fb540

Previous changes used the docstring of the test_generation module, which does not inform a user about the script. Signed-off-by: Werner Lewis <[email protected]>

minosgalanakis reviewed Sep 16, 2022

View reviewed changes

minosgalanakis approved these changes Sep 16, 2022

View reviewed changes

gilles-peskine-arm approved these changes Sep 16, 2022

View reviewed changes

gilles-peskine-arm reviewed Sep 16, 2022

View reviewed changes

gilles-peskine-arm merged commit 1716f06 into Mbed-TLS:development Sep 17, 2022

gilles-peskine-arm mentioned this pull request Sep 18, 2022

Minor fixes to test_data_generation.py #6296

Merged

wernerlewis mentioned this pull request Sep 21, 2022

[Backport 2.28] Add bignum test case generation script #6307

Merged

wernerlewis added needs-backports Backports are missing or are pending review and approval. and removed needs-review Every commit must be reviewed by at least two team members, labels Sep 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bignum test case generation script #6093

Add bignum test case generation script #6093

wernerlewis commented Jul 18, 2022 •

edited

Loading

minosgalanakis left a comment

minosgalanakis Aug 22, 2022

gilles-peskine-arm Aug 22, 2022

yanesca Aug 23, 2022

gilles-peskine-arm Aug 23, 2022

yanesca Aug 24, 2022

gilles-peskine-arm Aug 24, 2022

yanesca Aug 24, 2022

gilles-peskine-arm Aug 24, 2022

tom-cosgrove-arm Aug 24, 2022

yanesca Aug 24, 2022

minosgalanakis left a comment

minosgalanakis Sep 13, 2022

gilles-peskine-arm Sep 13, 2022

minosgalanakis Sep 13, 2022

minosgalanakis Sep 13, 2022

gilles-peskine-arm Sep 13, 2022

minosgalanakis Sep 16, 2022

gilles-peskine-arm left a comment

gilles-peskine-arm Sep 13, 2022

gilles-peskine-arm Sep 13, 2022

minosgalanakis Sep 16, 2022 •

edited

Loading

minosgalanakis left a comment

gilles-peskine-arm left a comment

gilles-peskine-arm Sep 16, 2022

gilles-peskine-arm Sep 18, 2022

gilles-peskine-arm Sep 16, 2022

gilles-peskine-arm Sep 16, 2022

gilles-peskine-arm Sep 16, 2022

		super().__init__(val_a.strip("-"), val_b.strip("-"))


		class BignumAdd(BignumOperation):

		The return value is cast, as older versions of mypy are unable to derive
		the specific type returned by itertools.combinations_with_replacement.

		@@ -0,0 +1,219 @@
		"""Common test generation classes and main function.

		from mbedtls_dev import build_tree
		from mbedtls_dev import test_case

Add bignum test case generation script #6093

Add bignum test case generation script #6093

Conversation

wernerlewis commented Jul 18, 2022 • edited Loading

Description

Status

Requires Backporting

minosgalanakis left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minosgalanakis left a 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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minosgalanakis Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

minosgalanakis left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a 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

wernerlewis commented Jul 18, 2022 •

edited

Loading

minosgalanakis Sep 16, 2022 •

edited

Loading