Add ssl_ctx_test to test suite.#10337
Add ssl_ctx_test to test suite.#10337richsalz wants to merge 1 commit intoopenssl:masterfrom richsalz:test_ssl_ctx
Conversation
|
Yup, I have run it, and think that's valuable to figure out what we do right or wrong. TDD (Test Driven Development) is a thing that we sometimes do too little of. So basically, either the ssl code isn't quite right, or the test isn't. Fixing one or the other based on the results shown in this PR should spark another PR or two. |
|
TDD is for branches, not master, IMO. :) |
|
Feature branches, yeah. But that's what I'm saying, the presence of this failing test, even though "just" in a PR, may be reason for someone wanting to fix the problem. Quite obviously, the fixing branch will have to be merged before this test is, to not disturb the nice green colour of master builds. |
|
The "quite obviously" part wasn't clear from your earlier comments. But WTH, master's been broken for weeks anyway. |
|
The error is most likely: #6553 (review) |
|
That test made sense for #5127, which tried to change the behaviour, but as pointed out, is not what we wanted. |
|
So that means we need to make some small adjustment to the test program. Would you be willing to, @kroeckx? |
|
On Sun, Nov 03, 2019 at 02:55:47AM -0800, Richard Levitte wrote:
So that means we need to make some small adjustment to the test program. Would you be willing to, @kroeckx?
It's just changing a 0 into a 1 on that line.
|
|
Not just that. Also the expected max value needs to be changed. |
|
@richsalz could you please fix the ssl_ctx_test.c accordingly? |
|
rebased and updated commit pushed. locally-run test passes. |
t8m
left a comment
There was a problem hiding this comment.
Please also adjust the commit message to somehow describe the ssl_ctx_test.c change - i.e. that setting min version > max version is allowed, just eventually using that context for connections should fail.
Or I can adjust the message when merging.
|
not sure what you're asking; feel free to edit the commit message yourself. |
Also fix the test as min version > max version is allowed because the API calls to set min and max versions are separate and there can be legitimately a temporary situation when the condition is true even with correctly working application. The failure in this condition will be detected only during a handshake attempt. Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #10337)
|
Merged to master as 3105535 |
This reverts commit 89a2668. The fix is not needed anymore, as the test have been changed to reflect that min is allowed to be larger than max. See link below for more information: openssl#10337
Did anyone try to build #6553? :)
And then run it? :(
This PR makes the test program compile. But it fails. What to do?
Ping the guilty parties: @paulidale, @levitte, @tiran.