fix(tests): poll for logs in smoke tests to handle CloudWatch ingestion latency#774
Merged
fix(tests): poll for logs in smoke tests to handle CloudWatch ingestion latency#774
Conversation
sid-rl
force-pushed
the
siddarth/logs-flake
branch
from
186ac63 to
35154c3
Compare
jrvb-rl
approved these changes
Mar 31, 2026
Contributor
jrvb-rl
left a comment
jrvb-rl
left a comment
There was a problem hiding this comment.
main changes here look reasonable. I'm curious about why you are removing some of these existing test, though.
Also, can you add a note to the first polling occurrence in each test file describing what polling behavior we think is needed? (eg, why 10x 1s polls instead of something else?). We don't need this every occurrence, but having this knowledge semi-discoverable is probably handy.
Comment on lines
-1043
to
-1056
| @pytest.mark.timeout(THIRTY_SECOND_TIMEOUT) | ||
| async def test_logs_basic(self, shared_devbox: AsyncDevbox) -> None: | ||
| """Test retrieving devbox logs returns valid response structure.""" | ||
| test_message = "async basic log test message" | ||
| result = await shared_devbox.cmd.exec(f'echo "{test_message}"') | ||
| assert result.exit_code == 0 | ||
|
|
||
| logs = await shared_devbox.logs() | ||
|
|
||
| assert logs is not None | ||
| assert hasattr(logs, "logs") | ||
| assert isinstance(logs.logs, list) | ||
| log_content = " ".join(str(log) for log in logs.logs) | ||
| assert test_message in log_content |
Contributor
There was a problem hiding this comment.
This is testing a slightly different path (filter by execution ID or no...) Is there a reason you got rid of this one?
Comment on lines
-1029
to
-1031
| @pytest.mark.timeout(THIRTY_SECOND_TIMEOUT) | ||
| def test_logs_basic(self, shared_devbox: Devbox) -> None: | ||
| """Test retrieving devbox logs returns valid response structure.""" |
jrvb-rl
approved these changes
Mar 31, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Poll for logs in smoke tests to handle CloudWatch ingestion latency. Consolidates
test_logs_basicintotest_logs_with_execution_filtersince the basic test adds no coverage beyond the filtered variant.