feature(ingestion-acceptance-tests): extend timeout logging#53812
Open
feature(ingestion-acceptance-tests): extend timeout logging#53812
Conversation
… still available at timeout
Contributor
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+150
to
152
| f":stopwatch: Timeout: {config.activity_timeout_seconds}s | " | ||
| f":key: Token: `{config.project_api_key}`" | ||
| ), |
Contributor
There was a problem hiding this comment.
config.project_api_key is a live ingestion credential being sent into a Slack message. Slack messages are persisted server-side, appear in search results, and can be forwarded or screen-shared — making this an unnecessary credential-leak surface. Even for a dedicated test project, the project API key can be used to ingest arbitrary events. Consider replacing this with a non-sensitive identifier (e.g., a truncated prefix like phc_...abc) or removing it entirely.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/temporal/ingestion_acceptance_test/slack.py
Line: 150-152
Comment:
**API key exposed in Slack notification**
`config.project_api_key` is a live ingestion credential being sent into a Slack message. Slack messages are persisted server-side, appear in search results, and can be forwarded or screen-shared — making this an unnecessary credential-leak surface. Even for a dedicated test project, the project API key can be used to ingest arbitrary events. Consider replacing this with a non-sensitive identifier (e.g., a truncated prefix like `phc_...abc`) or removing it entirely.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+199
to
+241
| class TestSnapshotWithPolls: | ||
| def test_correlates_tests_with_pending_polls_by_thread_id(self) -> None: | ||
| rt = RunningTests() | ||
| rt._tests = {100: "TestA::test_one", 200: "TestB::test_two"} | ||
|
|
||
| mock_client = MagicMock() | ||
| mock_client.pending_polls_snapshot.return_value = { | ||
| 100: "event UUID 'abc-123'", | ||
| 200: "person with distinct_id 'xyz-456'", | ||
| } | ||
|
|
||
| result = rt.snapshot_with_polls(mock_client) | ||
|
|
||
| assert result == [ | ||
| RunningTestInfo(name="TestA::test_one", pending_poll="event UUID 'abc-123'"), | ||
| RunningTestInfo(name="TestB::test_two", pending_poll="person with distinct_id 'xyz-456'"), | ||
| ] | ||
|
|
||
| def test_pending_poll_is_none_when_thread_has_no_active_poll(self) -> None: | ||
| rt = RunningTests() | ||
| rt._tests = {100: "TestA::test_one", 200: "TestB::test_two"} | ||
|
|
||
| mock_client = MagicMock() | ||
| mock_client.pending_polls_snapshot.return_value = { | ||
| 100: "event UUID 'abc-123'", | ||
| } | ||
|
|
||
| result = rt.snapshot_with_polls(mock_client) | ||
|
|
||
| assert result == [ | ||
| RunningTestInfo(name="TestA::test_one", pending_poll="event UUID 'abc-123'"), | ||
| RunningTestInfo(name="TestB::test_two", pending_poll=None), | ||
| ] | ||
|
|
||
| def test_returns_empty_list_when_no_tests_running(self) -> None: | ||
| rt = RunningTests() | ||
|
|
||
| mock_client = MagicMock() | ||
| mock_client.pending_polls_snapshot.return_value = {} | ||
|
|
||
| result = rt.snapshot_with_polls(mock_client) | ||
|
|
||
| assert result == [] |
Contributor
There was a problem hiding this comment.
The three TestSnapshotWithPolls tests share identical structure (set up _tests, set up pending_polls_snapshot, call snapshot_with_polls, assert result) with only inputs and expected output varying. Per the project's testing conventions, this should be a single @pytest.mark.parametrize test. The same pattern applies to test_payload_renders_running_tests_with_poll_descriptions and test_payload_renders_running_test_without_poll_when_none in test_slack.py.
@pytest.mark.parametrize(
"tests_by_tid, polls_by_tid, expected",
[
(
{100: "TestA::test_one", 200: "TestB::test_two"},
{100: "event UUID 'abc-123'", 200: "person with distinct_id 'xyz-456'"},
[RunningTestInfo("TestA::test_one", "event UUID 'abc-123'"), RunningTestInfo("TestB::test_two", "person with distinct_id 'xyz-456'")],
),
(
{100: "TestA::test_one", 200: "TestB::test_two"},
{100: "event UUID 'abc-123'"},
[RunningTestInfo("TestA::test_one", "event UUID 'abc-123'"), RunningTestInfo("TestB::test_two", None)],
),
({}, {}, []),
],
)
def test_snapshot_with_polls(self, tests_by_tid, polls_by_tid, expected) -> None:
rt = RunningTests()
rt._tests = tests_by_tid
mock_client = MagicMock()
mock_client.pending_polls_snapshot.return_value = polls_by_tid
assert rt.snapshot_with_polls(mock_client) == expectedPrompt To Fix With AI
This is a comment left during a code review.
Path: posthog/temporal/tests/ingestion_acceptance_test/test_runner.py
Line: 199-241
Comment:
**Prefer parameterised tests**
The three `TestSnapshotWithPolls` tests share identical structure (set up `_tests`, set up `pending_polls_snapshot`, call `snapshot_with_polls`, assert result) with only inputs and expected output varying. Per the project's testing conventions, this should be a single `@pytest.mark.parametrize` test. The same pattern applies to `test_payload_renders_running_tests_with_poll_descriptions` and `test_payload_renders_running_test_without_poll_when_none` in `test_slack.py`.
```python
@pytest.mark.parametrize(
"tests_by_tid, polls_by_tid, expected",
[
(
{100: "TestA::test_one", 200: "TestB::test_two"},
{100: "event UUID 'abc-123'", 200: "person with distinct_id 'xyz-456'"},
[RunningTestInfo("TestA::test_one", "event UUID 'abc-123'"), RunningTestInfo("TestB::test_two", "person with distinct_id 'xyz-456'")],
),
(
{100: "TestA::test_one", 200: "TestB::test_two"},
{100: "event UUID 'abc-123'"},
[RunningTestInfo("TestA::test_one", "event UUID 'abc-123'"), RunningTestInfo("TestB::test_two", None)],
),
({}, {}, []),
],
)
def test_snapshot_with_polls(self, tests_by_tid, polls_by_tid, expected) -> None:
rt = RunningTests()
rt._tests = tests_by_tid
mock_client = MagicMock()
mock_client.pending_polls_snapshot.return_value = polls_by_tid
assert rt.snapshot_with_polls(mock_client) == expected
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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
1 participant
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.
Problem
Fix a small bug that prevents reporting in-flight jobs from snapshot when the job times out awaiting expected events.
Changes
How did you test this code?
Locally and in CI (new unit tests too)
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Publish to changelog?
N/A
Docs update
N/A