Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: BatchExport tests #17549

Closed
wants to merge 3 commits into from
Closed

Conversation

tomasfarias
Copy link
Contributor

@tomasfarias tomasfarias commented Sep 20, 2023

Problem

While working on #17535, I noticed a lot of duplication and boilerplate code in batch export tests. This makes it hard to identify what the test is about, and adding new tests requires a lot of copying and pasting.

Changes

  • Extended insert_events to support generating test events, duplicate events, events from different teams, and events outside the export range. The events desired can be configured by function caller.
  • Replaced all boilerplate event generation in unit tests with a call to insert_events.
  • Added fixtures for organization, team, batch export, temporal client, and clickhouse client creation.
  • Replaced all team, organization, batch export, temporal client and clickhouse client creation from tests with the new fixtures.
  • Moved "fixtures" in fixtures.py to base.py as they are not fixtures, just helper test functions.
  • Deleted fixtures.py as fixtures should be in conftest.py.
  • Fixed a bug spotted by the new tests happening when specifying a kms_key_id with an encryption different from aws:kms (although impossible via the frontend, so not critical).

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

The code is tests.

@tomasfarias tomasfarias marked this pull request as draft September 20, 2023 14:08
@tomasfarias tomasfarias force-pushed the refactor/batch-exports-tests branch from 098a073 to fed0e20 Compare September 20, 2023 14:12
@tomasfarias tomasfarias changed the title feat: Record S3 BatchExport errors refactor: BatchExport tests for S3 Sep 20, 2023
Base automatically changed from feat/report-lastest-batch-export-error to master September 22, 2023 13:35
@tomasfarias tomasfarias changed the title refactor: BatchExport tests for S3 refactor: BatchExport tests Sep 22, 2023
@tomasfarias tomasfarias force-pushed the refactor/batch-exports-tests branch from fed0e20 to f860144 Compare September 22, 2023 13:40
@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@posthog-bot posthog-bot closed this Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants