Skip to content

[TLS 1.3] Add a "consolidation c'tor" for Test::Result#2967

Merged
reneme merged 1 commit intomasterfrom
tls13/test_result_merging
Jun 1, 2022
Merged

[TLS 1.3] Add a "consolidation c'tor" for Test::Result#2967
reneme merged 1 commit intomasterfrom
tls13/test_result_merging

Conversation

@reneme
Copy link
Copy Markdown
Collaborator

@reneme reneme commented Apr 27, 2022

This is somewhat a request for comment in the hope to iterate towards a better solution.

For the RFC 8448 based test in the main TLS pull request, I would like to use the new CHECK helper to structure sub sections of individual tests. Furthermore, I want to use Text_Based_Tests to store the transcript records of RFC 8448.

Now, using CHECK results in a vector of Test::Result objects but Text_Based_Tests::run_one_test expects a single Test::Result as return value. Hence, I added a "consolidation constructor" to somewhat merge the fine grained downstream Test::Result objects into a single Test::Result to return into the test framework. I'm not super happy with this approach, as I'm not sure whether it swallows vital failure information that might make test debugging harder.

@reneme reneme changed the title Add a "consolidation c'tor" for Test::Result [TLS 1.3] Add a "consolidation c'tor" for Test::Result Apr 27, 2022
@reneme reneme mentioned this pull request Apr 27, 2022
Merged
16 tasks
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 27, 2022 •
edited
Loading

Codecov Report

Merging #2967 (a00bf4c) into master (2fb7ba8) will decrease coverage by 0.01%.
The diff coverage is 37.50%.

@@            Coverage Diff             @@
##           master    #2967      +/-   ##
==========================================
- Coverage   92.56%   92.55%   -0.02%     
==========================================
  Files         577      577              
  Lines       66736    66739       +3     
  Branches     6384     6386       +2     
==========================================
- Hits        61777    61767      -10     
- Misses       4926     4939      +13     
  Partials       33       33
Impacted Files Coverage Δ
src/tests/tests.cpp 82.12% <28.57%> (-0.79%) ⬇️
src/tests/tests.h 95.62% <100.00%> (ø)
src/lib/pbkdf/argon2/argon2pwhash.cpp 77.46% <0.00%> (-7.05%) ⬇️
src/lib/pubkey/mce/mceliece_key.cpp 83.24% <0.00%> (-1.05%) ⬇️
src/tests/test_tls_messages.cpp 90.00% <0.00%> (-0.91%) ⬇️
src/tests/test_clang_bug.cpp 92.85% <0.00%> (-0.48%) ⬇️
src/tests/test_certstor_flatfile.cpp 87.91% <0.00%> (-0.09%) ⬇️
src/tests/test_ffi.cpp 99.68% <0.00%> (-0.07%) ⬇️
src/tests/unit_asio_stream.cpp 99.35% <0.00%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fb7ba8...a00bf4c. Read the comment docs.

@reneme
Copy link
Copy Markdown
Collaborator Author

reneme commented Apr 27, 2022 •
edited
Loading

For reference, a realistic failure in a downstream Test::Result looks about like that:

tls_rfc8448:
Client Authentication (Client side) ran 4 tests in 2.74 msec 2 FAILED
Failure 1: other handshake messages and client auth failed unexpectedly with error Test error Fixed output RNG ran out of bytes, test bug?
Failure 2: Close Connection unexpected result for Client close_notify
Produced: 1703030013BD3A77FF4BD1F015E4ECB853A0E6B392981B2A
Expected: 1703030013E4AD7D44C29245339D355962C779B89EF44C58
XOR Diff: 000000000059970ABB8943B52679D9E131679F0B0C6C5772
Note 1: Client Authentication (Client side) Test # 1 Client_Authentication_Handshake failed ClientFinished=170303... ClientHello_1=160301... Client_CloseNotify=170303... RNG_Pool=6a47... ServerHandshakeMessages=170303... ServerHello=160303... Server_CloseNotify=170303...

The error was happening while developing this test case:

botan/src/tests/test_tls_rfc8448.cpp

Line 815 in 3abfc13

static std::vector<Test::Result> client_authentication(const VarMap& vars)

@randombit
Copy link
Copy Markdown
Owner

randombit commented Jun 1, 2022

Seems kind of hackish but I don't have an immediate suggestion on improvement and I do see the need for this functionality, so I'm fine with this PR as is.

@reneme reneme marked this pull request as ready for review June 1, 2022 11:42
@reneme
Copy link
Copy Markdown
Collaborator Author

reneme commented Jun 1, 2022

Seems kind of hackish but I don't have an immediate suggestion on improvement and I do see the need for this functionality, so I'm fine with this PR as is.

Yep, indeed the solution is hacky. I'll keep a reference of it in the "modernize test framework" discussion thread: #2883 (comment)

This PR was a "draft pull request" until a minute ago. Please approve or merge. :)

@reneme reneme requested a review from randombit June 1, 2022 12:01
@reneme reneme merged commit f4bbe0a into master Jun 1, 2022
@reneme reneme deleted the tls13/test_result_merging branch June 7, 2022 07:58
@reneme reneme mentioned this pull request Aug 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@randombit randombit randombit approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@reneme @codecov-commenter @randombit