test: more readable logging for smoke tests #5099
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
Moving the smoke test invocations out of the process-compose wrapper, preferring running raw `cargo test` invocations instead. Doing so ensures that the test output is readable, both locally, and crucially in CI.
conorsch
force-pushed
the
smoke-test-readable-output
branch
from
0a67537 to
7a71867
Compare
conorsch
commented
Feb 19, 2025
| @@ -39,9 +40,9 @@ const TIMEOUT_COMMAND_SECONDS: u64 = 20; | |||
| // The "unbonding_delay" value is specified in blocks, and in the smoke tests, | |||
| // block time is set to ~500ms, so we'll take the total number of blocks | |||
| // that must elapse and sleep half that many seconds. | |||
| static UNBONDING_DURATION: Lazy<Duration> = Lazy::new(|| { | |||
| static UNBONDING_DELAY: Lazy<Duration> = Lazy::new(|| { | |||
There was a problem hiding this comment.
Renamed for consistency, so the value is greppable throughout the tests.
| .unwrap_or_else(|_| "http://127.0.0.1:8080".to_owned()) | ||
| .parse() | ||
| .expect("failed to parse PENUMBRA_NODE_PD_URL"); | ||
|
|
There was a problem hiding this comment.
Added a sane default for which grpc node pcli should talk to, same as already exists in the other tests. This value takes precedence when no env var override exists.
| @@ -36,8 +36,11 @@ else | |||
| use_tui="false" | |||
| fi | |||
|
|
|||
| # Set unique API port for controlling running services. | |||
| export PC_PORT_NUM="8888" | |||
|
|
|||
There was a problem hiding this comment.
TODO: we can and should use unix sockets where possible, but I'm punting on that for now, and just using 8888/TCP for the compose API port.
| let blocks: f64 = std::env::var("UNBONDING_DELAY") | ||
| .unwrap_or("100".to_string()) | ||
| .unwrap_or("50".to_string()) |
There was a problem hiding this comment.
Adjusted default value to match what we being set in the smoke tests, via env var override.
cronokirby
approved these changes
Feb 19, 2025
cronokirby
approved these changes
Feb 19, 2025
2 tasks
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.
Describe your changes
Moving the smoke test invocations out of the process-compose wrapper, preferring running raw
cargo testinvocations instead. Doing so ensures that the test output is readable, both locally, and crucially in CI.Screenshot before
Screenshot after
Issue ticket number and link
No guiding issue, just trying to improve the testing setup opportunistically. We did have inscrutable smoke test failures on #5063 & #5081, both of which were failures of the new pindexer integration tests (#5057), but it was hard to see the specific failure at a glance, which slowed down development.
Testing and review
Check the CI logs for the smoke test job. Are they readable? Consider running
just smokelocally and confirm the same.Checklist before requesting a review
I have added guiding text to explain how a reviewer should test these changes.
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: