bindings: clean up blinding tests #4356
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maddeleine
reviewed
Jan 11, 2024
lrstewart
force-pushed
the
blinding_tests
branch
from
890c00d
to
6556317
Compare
jmayclin
reviewed
Jan 11, 2024
| // write the close_notify message that the server needs. | ||
| // Because time is mocked for testing, this does not actually take 10 minutes. | ||
| // TODO: replace this with a half-close once the bindings support half-close. | ||
| _ = time::timeout(time::Duration::from_secs(600), client.shutdown()).await; |
There was a problem hiding this comment.
Just so that things are in a known state, could we assert on the expected result here? Also why 600 seconds?
There was a problem hiding this comment.
Also why 600 seconds?
Just arbitrary wild overkill. If sending one messages takes more than 10min, you either have deeper problems or your environment is so broken as to be useless :) The mocked time makes wild overkill cheap.
jmayclin
approved these changes
Jan 16, 2024
maddeleine
approved these changes
Jan 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolved issues:
Resolves #3976
Description of changes:
handshake_error_with_blinding sometimes failed. The sequence of events was:
handshake_error_with_blinding calls shutdown twice, once with a timeout of 10s and once with a timeout of 30s. That gives shutdown a total of 40s to complete both blindings. I'm honestly surprised it doesn't fail more often :)
I've rewritten the tests to be clearer and less reliant on timeouts. Each test now specifically times the execution of s2n_shutdown and makes assertions about its execution time and result. I'm also specifically checking for the correct errors to ensure we're not overlooking more issues like #4350.
I could also check that the time_elapsed doesn't exceed some upper bound (40s maybe? max blinding + safety margin for actual work), but I worry that has the potential to continue to be flaky if the environment introduces some delay. I think the combo of checking error types + testing both sources of blinding separately makes me pretty confident.
Callouts:
This PR cleans up the tests, but does not fix #4350. They're really two different problems, so I'd like to fix them separately and not split reviewers' attention.
Testing:
This change is nothing but tests.
I'm having trouble consistently reproing the flaky test failure in CI, but it failed frequently on my mac. After this change, I have seen zero failures on my mac, even after leaving it running in a loop.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.