-
Notifications
You must be signed in to change notification settings - Fork 12
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
Refactor Tests Script for preparing ACVP python script Integration #234
Conversation
7005190
to
aaedf55
Compare
4379e03
to
70495b1
Compare
tests all
not working if tests
is not in PATH and add suffix to binaries to allow building opt/non-opt binaries at the same time
70495b1
to
45518b6
Compare
631ff9e
to
3623250
Compare
tests all
not working if tests
is not in PATH and add suffix to binaries to allow building opt/non-opt binaries at the same time821b530
to
dbbdef2
Compare
482679c
to
bdbf7fd
Compare
bdbf7fd
to
7eadeb2
Compare
7eadeb2
to
453f5a4
Compare
453f5a4
to
659cd88
Compare
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.
Thanks @potsrevennil for the refactoring - this looks cleaner to me.
I still need to go through the changes in detail, but maybe you can already have a look at this:
$ tests bench --opt all
does not work the way I would expect it - it only runs the opt tests.
3a652ce
to
d334dbc
Compare
…her one of it Signed-off-by: Thing-han, Lim <[email protected]>
Tests are bounded by IO, so running tests asynchronously doesn't add value, instead add more complexity only. Therefore avoiding running test binaries with asyncio for simplicity Signed-off-by: Thing-han, Lim <[email protected]>
…script Signed-off-by: Thing-han, Lim <[email protected]>
Make logging format look nicer by utilizing the logger name, decorated by test type, scheme, compile mode and implementation Signed-off-by: Thing-han, Lim <[email protected]>
`State` class naming was a mistake, renaming it to Options And refactor the member functions of State into member functions of `Base`, `Test_Implementations`, `Tests` `Base`: the lowest level class for handling compilation and binary execution `Test_Implementations`: handle running one or multiple implementations `Tests`: provide top level functions for click cli commands Signed-off-by: Thing-han, Lim <[email protected]>
d334dbc
to
e2aa705
Compare
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.
Thanks for the updates @potsrevennil.
This still doesn't do what I'd expect it to do: nevermind - I was running an old version - let me try again.
$ tests bench --opt all --cycles PERF
None
INFO > Benchmark Compile (native, opt) make CROSS_PREFIX= bench CYCLES=PERF OPT=1 AUTO=1
INFO > Benchmark ML-KEM-512 (native, opt) keypair cycles = 24255
encaps cycles = 30724
decaps cycles = 39536
percentile 1 10 20 30 40 50 60 70 80 90 99
keypair percentiles: 23877 23975 24052 24114 24162 24255 24338 24450 24615 24919 26850
encaps percentiles: 30305 30454 30519 30590 30652 30724 30815 30929 31134 31487 35613
decaps percentiles: 39076 39228 39304 39393 39456 39536 39640 39727 39907 40208 42464
INFO > Benchmark ML-KEM-768 (native, opt) keypair cycles = 43647
encaps cycles = 47745
decaps cycles = 60480
percentile 1 10 20 30 40 50 60 70 80 90 99
keypair percentiles: 41991 42248 42475 42824 43240 43647 44009 44328 44817 45580 48292
encaps percentiles: 45859 46158 46361 46649 47284 47745 48064 48347 48944 49807 52009
decaps percentiles: 58633 58936 59164 59469 60010 60480 60869 61219 61821 62552 64655
INFO > Benchmark ML-KEM-1024 (native, opt) keypair cycles = 62950
encaps cycles = 69488
decaps cycles = 87193
percentile 1 10 20 30 40 50 60 70 80 90 99
keypair percentiles: 60969 61673 62105 62409 62658 62950 63238 63565 64200 64894 67050
encaps percentiles: 67655 68272 68630 68972 69238 69488 69886 70193 70607 71357 73030
decaps percentiles: 85259 85922 86317 86630 86906 87193 87540 87929 88307 89070 90809
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.
Thanks @potsrevennil - this looks good though.
Hopefully the last rework of the entire test script.
One downside is that we now no longer check the namespacing for the no-opt implementation in the quickcheck CI. Let's do that in a follow-up. I opened an issue #372
Refactor
tests
script mainly for preparation to easily integrate acvp intotests
:scripts/tests
script now acts as a Click-based CLI interface.scripts/lib/test.py
, providing APIs that the Click commands call for main testing functionality.State
class as mentioned in https://github.com/pq-code-package/mlkem-c-aarch64/pull/347#discussion_r1830477719)tests func/kat/nistkat/all
interface consistent:--opt
flags become choice ofALL | OPT | NO_OPT
(case insensitive)