test(e2e): Actually run Celery tasks async in `e2e-test-runner` #23973

Twixes · 2024-07-25T10:27:01Z

Problem

Adding accurate E2E tests for behavior relying on Celery tasks hasn't been possible, as bin/e2e-test-runner was actually running PostHog in TEST and DEBUG mode, resulting in non-production-like behavior, e.g. Celery tasks being always executed sync (CELERY_ALWAYS_EAGER setting) or Django cache being in-memory rather than over Redis.

With async insight execution being critical now (e.g. see #23808), we need to be able to test it.

Changes

This makes a few changes to the setup of bin/e2e-test-runner so that PostHog behavior mirrors production. This has allowed adding a test for async insight execution, that actually verifies execution being kicked off, and polled results becoming soon available.

It doesn't look like significant changes to the CI workflow are necessary, as it doesn't set TEST=1 or DEBUG=1 and already starts bin/docker-worker.

github-actions · 2024-07-25T15:41:34Z

Size Change: 0 B

Total Size: 1.07 MB

ℹ️ View Unchanged

Filename	Size
`frontend/dist/toolbar.js`	1.07 MB

_{compressed-size-action}

skoob13 · 2024-07-30T09:34:56Z

@Twixes, I tried to run the command bin/e2e-test-runner, but I got the error below. The same script works fine on master. Did I do something wrong?

2024-07-30 11:24:43 [warning  ] ️WARNING! Environment variable E2E_TESTING is enabled. This is a security vulnerability unless you are running tests.
{'event': ['Skipping service version requirements. This is dangerous and PostHog might not work as expected!'], 'timestamp': '2024-07-30T09:24:43.758175Z', 'logger': 'posthog.settings.service_requirements', 'level': 'warning', 'pid': 22167, 'tid': 8423623680}
2024-07-30T09:24:46.639066Z [warning  ] Skipping async migrations setup. This is unsafe in production! [posthog.apps] pid=22167 tid=8423623680
2024-07-30T09:24:46.640829Z [info     ] AXES: BEGIN LOG                [axes.apps] pid=22167 tid=8423623680
2024-07-30T09:24:46.641167Z [info     ] AXES: Using django-axes version 5.9.0 [axes.apps] pid=22167 tid=8423623680
2024-07-30T09:24:46.641210Z [info     ] AXES: blocking by IP only.     [axes.apps] pid=22167 tid=8423623680
Traceback (most recent call last):
  File "/Users/ggg/Projects/posthog/posthog/env/lib/python3.11/site-packages/infi/clickhouse_orm/database.py", line 366, in _get_applied_migrations_and_create_tables
    return self._get_applied_migrations(migrations_package_name, replicated)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ggg/Projects/posthog/posthog/env/lib/python3.11/site-packages/infi/clickhouse_orm/database.py", line 383, in _get_applied_migrations
    return set(obj.module_name for obj in self.select(query))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ggg/Projects/posthog/posthog/env/lib/python3.11/site-packages/infi/clickhouse_orm/database.py", line 383, in <genexpr>
    return set(obj.module_name for obj in self.select(query))
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ggg/Projects/posthog/posthog/env/lib/python3.11/site-packages/infi/clickhouse_orm/database.py", line 280, in select
    r = self._send(query, settings, True)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ggg/Projects/posthog/posthog/env/lib/python3.11/site-packages/infi/clickhouse_orm/database.py", line 393, in _send
    raise ServerError(r.text)
infi.clickhouse_orm.database.ServerError: Code: 60. DB::Exception: Table posthog_test.infi_clickhouse_orm_migrations_distributed does not exist. Maybe you meant default.infi_clickhouse_orm_migrations_distributed?. (UNKNOWN_TABLE) (version 23.12.5.81 (official build))
 (0)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/ggg/Projects/posthog/posthog/manage.py", line 22, in <module>
    main()
  File "/Users/ggg/Projects/posthog/posthog/manage.py", line 18, in main
    execute_from_command_line(sys.argv)
  File "/Users/ggg/Projects/posthog/posthog/env/lib/python3.11/site-packages/django/core/management/__init__.py", line 442, in execute_from_command_line
    utility.execute()
  File "/Users/ggg/Projects/posthog/posthog/env/lib/python3.11/site-packages/django/core/management/__init__.py", line 436, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/Users/ggg/Projects/posthog/posthog/env/lib/python3.11/site-packages/django/core/management/base.py", line 412, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/Users/ggg/Projects/posthog/posthog/env/lib/python3.11/site-packages/django/core/management/base.py", line 458, in execute
    output = self.handle(*args, **options)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ggg/Projects/posthog/posthog/posthog/management/commands/migrate_clickhouse.py", line 53, in handle
    self.migrate(CLICKHOUSE_HTTP_URL, options)
  File "/Users/ggg/Projects/posthog/posthog/posthog/management/commands/migrate_clickhouse.py", line 92, in migrate
    database.migrate(MIGRATIONS_PACKAGE_NAME, options["upto"], replicated=True)
  File "/Users/ggg/Projects/posthog/posthog/env/lib/python3.11/site-packages/infi/clickhouse_orm/database.py", line 350, in migrate
    applied_migrations = self._get_applied_migrations_and_create_tables(migrations_package_name, replicated=replicated)
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ggg/Projects/posthog/posthog/env/lib/python3.11/site-packages/infi/clickhouse_orm/database.py", line 370, in _get_applied_migrations_and_create_tables
    self.create_table(MigrationHistoryReplicated)
  File "/Users/ggg/Projects/posthog/posthog/env/lib/python3.11/site-packages/infi/clickhouse_orm/database.py", line 156, in create_table
    self._send(model_class.create_table_sql(self))
  File "/Users/ggg/Projects/posthog/posthog/env/lib/python3.11/site-packages/infi/clickhouse_orm/database.py", line 393, in _send
    raise ServerError(r.text)
infi.clickhouse_orm.database.ServerError: Code: 253. DB::Exception: There was an error on [localhost:9000]: Code: 253. DB::Exception: Cannot create table, because it is created concurrently every time or because of wrong zookeeper_path or because of logical error. (REPLICA_ALREADY_EXISTS) (version 23.12.5.81 (official build)). (REPLICA_ALREADY_EXISTS) (version 23.12.5.81 (official build))
 (0)
[1]    22141 terminated  bin/e2e-test-runner

Twixes · 2024-07-30T12:41:51Z

Good shout @skoob13, should be fixed by 41d6bbf.

This was pretty bad - local E2E behavior was much different from actual app behavior.

anirudhpillai · 2024-07-30T13:46:03Z

bin/e2e-test-runner

@@ -103,4 +102,6 @@ $SKIP_SETUP_DEV || setupDev
 # Only start webpack if not already running
 nc -vz 127.0.0.1 8234 2> /dev/null || ./bin/start-frontend &
 pnpm dlx cypress open --config-file cypress.e2e.config.ts &
+uv pip install -r requirements.txt -r requirements-dev.txt


I had to install uv locally, maybe we should add this to requirements-dev.txt so that all the guides like 'Developing locally` work out of the box?

I think the point of uv is to be global, like pnpm/npm or other package managers, so that's why we don't include it in the dependency list.

anirudhpillai

💯

skoob13 · 2024-07-30T14:13:30Z

@Twixes works well now. One question: should we pre-populate the test DB with test data?

Previously, we did have data:

With these changes, there is no default data, so it's somewhat hard to test cases like breakdowns where we need at least a few data points:

Twixes · 2024-07-31T10:45:00Z

That's really weird @skoob13, there should still be data (via the setupDev function in the script). That hasn't changed. 🤔

Twixes · 2024-07-31T13:33:26Z

Tried locally and everything worked, so I believe that might have been a fluke Georgiy. #BiasForImpact

Twixes force-pushed the celery-in-e2e branch from 2405664 to 35d2a2e Compare July 25, 2024 15:31

Twixes changed the base branch from master to revert-23977-revert-23808-fix/insight-single-load-cached July 25, 2024 15:31

Twixes marked this pull request as ready for review July 29, 2024 14:13

Twixes requested a review from a team July 29, 2024 14:13

anirudhpillai force-pushed the revert-23977-revert-23808-fix/insight-single-load-cached branch from dc2c84e to be64bf2 Compare July 29, 2024 18:29

Twixes added 10 commits July 30, 2024 14:53

Allow autoreloading Celery beat worker too

0453ef0

Disable DEBUG and TEST in e2e-test-runner

f66350f

This was pretty bad - local E2E behavior was much different from actual app behavior.

Add Celery worker to e2e-test-runner

b86314e

Account for query statuses in resetInsightCache

07dec3a

Create insights-saved.cy.ts

198086e

Fix some warning/error logging

9789cbe

Reset insight cache before every E2E suite

23e2fd7

Update CLOUD_DEPLOYMENT var

fc04419

Use Cypress chainable over Promise

97eb5bd

Still randomize replica paths in E2E tests

41d6bbf

Twixes force-pushed the celery-in-e2e branch 3 times, most recently from ad76b60 to 41d6bbf Compare July 30, 2024 12:55

PostHog deleted a comment from posthog-bot Jul 30, 2024

anirudhpillai reviewed Jul 30, 2024

View reviewed changes

anirudhpillai approved these changes Jul 30, 2024

View reviewed changes

Twixes merged commit 6fe1570 into revert-23977-revert-23808-fix/insight-single-load-cached Jul 31, 2024
94 of 97 checks passed

Twixes deleted the celery-in-e2e branch July 31, 2024 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(e2e): Actually run Celery tasks async in `e2e-test-runner` #23973

test(e2e): Actually run Celery tasks async in `e2e-test-runner` #23973

Twixes commented Jul 25, 2024

github-actions bot commented Jul 25, 2024 •

edited

Loading

skoob13 commented Jul 30, 2024

Twixes commented Jul 30, 2024 •

edited

Loading

anirudhpillai Jul 30, 2024

skoob13 Jul 30, 2024 •

edited

Loading

anirudhpillai left a comment

skoob13 commented Jul 30, 2024

Twixes commented Jul 31, 2024

Twixes commented Jul 31, 2024

test(e2e): Actually run Celery tasks async in e2e-test-runner #23973

test(e2e): Actually run Celery tasks async in e2e-test-runner #23973

Conversation

Twixes commented Jul 25, 2024

Problem

Changes

github-actions bot commented Jul 25, 2024 • edited Loading

skoob13 commented Jul 30, 2024

Twixes commented Jul 30, 2024 • edited Loading

anirudhpillai Jul 30, 2024

Choose a reason for hiding this comment

skoob13 Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

anirudhpillai left a comment

Choose a reason for hiding this comment

skoob13 commented Jul 30, 2024

Twixes commented Jul 31, 2024

Twixes commented Jul 31, 2024

test(e2e): Actually run Celery tasks async in `e2e-test-runner` #23973

test(e2e): Actually run Celery tasks async in `e2e-test-runner` #23973

github-actions bot commented Jul 25, 2024 •

edited

Loading

Twixes commented Jul 30, 2024 •

edited

Loading

skoob13 Jul 30, 2024 •

edited

Loading