-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add bignum test case generation script #6093
Changes from 8 commits
8b2df74
69a92ce
86caf85
a51fe2b
b17ca8a
c442f6a
265e051
6a31396
75ef944
383461c
fbb75e3
55e638c
92c876a
6c70d74
169034a
699e126
2b527a3
cfd4768
d03d2a3
6300b4f
9990b30
a195ce7
6d654c6
e3ad22e
a16b617
f156c43
6ef5436
9df9faa
76f4562
3366ebc
81f2444
a4b7720
466f036
aaf3b79
a4668a6
5601308
855e45c
1fade8a
3dc4519
34d6d3e
858cffd
00d0242
b6e8091
ac446c8
52ae326
07c830c
c2fb540
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,11 +6,11 @@ | |
|
||
Class structure: | ||
|
||
minosgalanakis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Child classes of test_generation.BaseTarget (file Targets) represent a target | ||
Child classes of test_generation.BaseTarget (file targets) represent an output | ||
file. These indicate where test cases will be written to, for all subclasses of | ||
this Target. Multiple Target classes should not reuse a `target_basename`. | ||
this target. Multiple file targets should not reuse a `target_basename`. | ||
|
||
Each subclass derived from a file Target can either be: | ||
Each subclass derived from a file target can either be: | ||
- A concrete class, representing a test function, which generates test cases. | ||
- An abstract class containing shared methods and attributes, not associated | ||
with a test function. An example is BignumOperation, which provides | ||
|
@@ -24,7 +24,7 @@ | |
Adding test case generation for a function: | ||
|
||
A subclass representing the test function should be added, deriving from a | ||
file Target such as BignumTarget. This test class must set/implement the | ||
file target such as BignumTarget. This test class must set/implement the | ||
following: | ||
- test_function: the function name from the associated .function file. | ||
- test_name: a descriptive name or brief summary to refer to the test | ||
|
@@ -56,9 +56,10 @@ | |
|
||
import itertools | ||
import sys | ||
import typing | ||
|
||
from abc import ABCMeta, abstractmethod | ||
from typing import Iterator, List, Tuple, TypeVar, cast | ||
from typing import Iterator, List, Tuple, TypeVar | ||
|
||
import scripts_path # pylint: disable=unused-import | ||
from mbedtls_dev import test_case | ||
|
@@ -72,6 +73,17 @@ def hex_to_int(val: str) -> int: | |
def quote_str(val) -> str: | ||
return "\"{}\"".format(val) | ||
|
||
def combination_pairs(values: List[T]) -> List[Tuple[T, T]]: | ||
"""Return all pair combinations from input values. | ||
|
||
The return value is cast, as older versions of mypy are unable to derive | ||
the specific type returned by itertools.combinations_with_replacement. | ||
Comment on lines
+79
to
+80
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #6296 has fixes for this and the other non-blocker issues I raised in this review. |
||
""" | ||
return typing.cast( | ||
List[Tuple[T, T]], | ||
list(itertools.combinations_with_replacement(values, 2)) | ||
) | ||
|
||
|
||
class BignumTarget(test_generation.BaseTarget, metaclass=ABCMeta): | ||
#pylint: disable=abstract-method | ||
|
@@ -99,7 +111,7 @@ class BignumOperation(BignumTarget, metaclass=ABCMeta): | |
"0000000000000000123", "-0000000000000000123", | ||
"1230000000000000000", "-1230000000000000000" | ||
] # type: List[str] | ||
input_cases = cast(List[Tuple[str, str]], []) # type: List[Tuple[str, str]] | ||
input_cases = [] # type: List[Tuple[str, str]] | ||
|
||
def __init__(self, val_a: str, val_b: str) -> None: | ||
self.arg_a = val_a | ||
|
@@ -164,10 +176,7 @@ def get_value_pairs(cls) -> Iterator[Tuple[str, str]]: | |
Combinations are first generated from all input values, and then | ||
specific cases provided. | ||
""" | ||
yield from cast( | ||
Iterator[Tuple[str, str]], | ||
itertools.combinations_with_replacement(cls.input_values, 2) | ||
) | ||
yield from combination_pairs(cls.input_values) | ||
yield from cls.input_cases | ||
|
||
@classmethod | ||
|
@@ -214,19 +223,16 @@ class BignumAdd(BignumOperation): | |
symbol = "+" | ||
test_function = "mbedtls_mpi_add_mpi" | ||
test_name = "MPI add" | ||
input_cases = cast( | ||
List[Tuple[str, str]], | ||
list(itertools.combinations_with_replacement( | ||
[ | ||
"1c67967269c6", "9cde3", | ||
"-1c67967269c6", "-9cde3", | ||
], 2 | ||
)) | ||
input_cases = combination_pairs( | ||
[ | ||
"1c67967269c6", "9cde3", | ||
"-1c67967269c6", "-9cde3", | ||
] | ||
) | ||
|
||
def result(self) -> str: | ||
return quote_str("{:x}".format(self.int_a + self.int_b)) | ||
|
||
|
||
if __name__ == '__main__': | ||
test_generation.main(sys.argv[1:]) | ||
# Use the section of the docstring relevant to the CLI as description | ||
test_generation.main(sys.argv[1:], "\n".join(__doc__.splitlines()[:4])) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is precicely why using "doc" was established. The same can be achieved by
This shall not block this PR, this is just for the purposes of discussion |
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.
Well, not quite.
--directory
is still interpreted relative to the root of the mbedtls tree. Anabspath
call is missing somewhere (or alternatively we could stop usingchdir
, 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.