Conversation
This is used with the pyca/cryptography test suite
paulidale
left a comment
paulidale
left a comment
There was a problem hiding this comment.
LGTM.
The run time is 13 minutes which is at the upper end of what we'd like but still okay.
paulidale
added
approval: review pending
|
We’re annoyed by runtime on our end too. There’s likely to be a patch to bypass RSA_check_key for (most) test cases at some point soon which will chop several minutes off wall clock time. |
|
Sounds good. We informally chose to limit CI runs to around 15 minutes when I added the non-caching build. This build is a couple of minutes shy of that figure, so it's not a concern at the moment and I doubt it will be for a while. |
mattcaswell
added
approval: done
slontis
left a comment
slontis
left a comment
There was a problem hiding this comment.
LGTM..
Just some aside comments about the test results.
There could be some cases where we support things now..
e.g KBKDF with cmac + RSA with truncated digests..
Thank you for the pointers to those features. It is our intent to revisit our skips and capability checks for 3.0.0 so I’ll keep those in mind as well. There are also a significant number of wycheproof tests we currently skip that 3.0.0 can now run. |
|
By the way, what do the appveyor CI jobs do that Actions does not? Is there a desire to consolidate on Actions? |
From the docs it looked like mac and windows consume more minutes (5 to 10 times). So I wasn't sure if it was a good idea to swap it over. Actions just does a minimal build currently to pick up anything stupid like compiler issues (and missing functions) that don't appear on other platforms. |
|
We've been discussing dropping the Appveyor builds in favour of Actions. None of the fellows have had the time to do this with 3.0 being more pressing. |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
paulidale
added
approval: ready to merge
Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Shane Lontis <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #15018)
This is used with the pyca/cryptography test suite Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Shane Lontis <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #15018)
Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Shane Lontis <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #15018)
Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Shane Lontis <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #15018)
|
Merged to master. Thanks for this! |
6 participants
Now that cryptography has merged 3.0.0 support OpenSSL can use its test suite again.
A few notes:
The current test harness you're using in Perl appears to be eating test output. That suppression should really be turned off so we can see things like which version it linked against, whether the wycheproof tests are loading as expected, etc. It's unpleasant to debug CI when the necessary details aren't available.