Remove giant write test #398
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
Motivation: The giant write test requires an enormous commitment of RAM to run it. This has caused problems for both our own CI and a number of downstream CI systems, leading to long execution times or outright failures. Unfortunately, we can't defend against the original failure using this test suite, but we can at least exercise the new machinery we added and trust that the configuration is right. Modifications: - Make the maximum write size configurable internally - Test the maximum write size at a different value. Result: We feel confident that the write splitting continues to work correctly.
dnadoba
approved these changes
Sep 1, 2022
| @@ -86,7 +86,7 @@ public final class NIOSSLClientHandler: NIOSSLHandler { | |||
| connection.setVerificationCallback(verificationCallback) | |||
| } | |||
|
|
|||
| super.init(connection: connection, shutdownTimeout: context.configuration.shutdownTimeout, additionalPeerCertificateVerificationCallback: nil) | |||
| super.init(connection: connection, shutdownTimeout: context.configuration.shutdownTimeout, additionalPeerCertificateVerificationCallback: nil, maxWriteSize: defaultMaxWriteSize) | |||
There was a problem hiding this comment.
Nit: maxWriteSize has the same default argument and therefore not needed here.
Suggested change
| super.init(connection: connection, shutdownTimeout: context.configuration.shutdownTimeout, additionalPeerCertificateVerificationCallback: nil, maxWriteSize: defaultMaxWriteSize) | |
| super.init(connection: connection, shutdownTimeout: context.configuration.shutdownTimeout, additionalPeerCertificateVerificationCallback: nil) |
There was a problem hiding this comment.
Note that this is a call to super.init, not to self.init, so it doesn't actually have the default argument.
Sources/NIOSSL/NIOSSLHandler.swift
Outdated
| /// BoringSSL. | ||
| /// | ||
| /// We have this default here instead of hardcoded into the software for testing purposes. | ||
| internal let defaultMaxWriteSize = Int(CInt.max) |
There was a problem hiding this comment.
Nit: I think it would be nicer if we make this a static property of NIOSSLHandler
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.
Motivation:
The giant write test requires an enormous commitment of RAM to run it.
This has caused problems for both our own CI and a number of downstream
CI systems, leading to long execution times or outright failures.
Unfortunately, we can't defend against the original failure using this
test suite, but we can at least exercise the new machinery we added and
trust that the configuration is right.
Modifications:
Result:
We feel confident that the write splitting continues to work correctly.
Resolves #388.