test(s2n-quic-dc): disable tracing by default for tests with large output #2571
Conversation
This file contains hidden or 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
d5a229b to
4ed7a98
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR disables tracing by default for tests with large output to reduce memory consumption and improve test stability. Key changes include:
- Downgrading tracing log levels from info to debug in key test helper functions.
- Introducing and integrating a new without_tracing function to conditionally disable tracing during tests.
- Adjusting runtime creation in tests to use a current-thread runtime and modifying test macros accordingly.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| dc/s2n-quic-dc/src/stream/tests/request_response.rs | Replaced info logging with debug logging and wrapped tests with without_tracing; updated runtime creation. |
| dc/s2n-quic-dc/src/testing.rs | Modified tracing initialization and added without_tracing function; updated default pooled value. |
| dc/s2n-quic-dc/src/stream/send/tests.rs | Updated test macros to use without_tracing in single-threaded test cases. |
| dc/s2n-quic-dc/src/path/secret/map/cleaner.rs | Prevented spawning cleaner thread in simulation mode. |
| dc/s2n-quic-dc/src/stream/testing.rs | Changed DEFAULT_POOLED and backlog values to better suit testing conditions. |
Comments suppressed due to low confidence (2)
dc/s2n-quic-dc/src/stream/tests/request_response.rs:436
- Changing the runtime builder from new_multi_thread() to new_current_thread() may affect tests that rely on concurrency; please verify that this change aligns with the intended test behavior.
fn new(harness: &Harness) -> Self {
dc/s2n-quic-dc/src/stream/send/tests.rs:290
- [nitpick] Consider adding explicit tests or logging assertions to ensure that multi-thread tests remain stable even when tracing is not disabled, given that without_tracing is not used in these scenarios.
// Since these run different threads, it's not super easy to use the `without_tracing` function.
WesleyRosenblum
approved these changes
Mar 27, 2025
dougch
pushed a commit
that referenced
this pull request
May 19, 2025
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.
Description of changes:
It looks like the issue in #2570 was due to the tracing logs consuming too much memory. This change fixes that issue by flagging certain tests that generate a lot of logs and disabling those by default. I've also turned down the log level from
debugtoinfoin CI so that should also help.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.