Skip to content

acc: Use granular locking in testserver#2816

Merged
denik merged 5 commits intomainfrom
denik/acc-granular-locking
May 7, 2025
Merged

acc: Use granular locking in testserver#2816
denik merged 5 commits intomainfrom
denik/acc-granular-locking

Conversation

@denik
Copy link
Copy Markdown
Contributor

@denik denik commented May 6, 2025

Changes

Why

This makes use of testserver.Server as safe as it was before but makes handlers run concurrently. Note, although most handlers run quickly, there is Delay option that sleeps in the handler when lock is acquired. With this change, sleep is done when lock is released.

This enables us to refactor acceptance test server to reuse single server rather than create a new listening socket & server per test, which adds more and more overhead as we add more tests.

This also enables more advanced usage of testserver.Server where handlers do more I/O.

Note, there is no consistent performance win or lose with the current test suite.

Tests

Ran test suite many times with hyperfine to ensure it's running stable.

@denik denik temporarily deployed to test-trigger-is May 6, 2025 09:41 — with GitHub Actions Inactive
@denik denik force-pushed the denik/acc-granular-locking branch from 02fcd1c to 9499abf Compare May 6, 2025 13:40
@denik denik temporarily deployed to test-trigger-is May 6, 2025 13:40 — with GitHub Actions Inactive
@denik denik marked this pull request as ready for review May 6, 2025 15:28
@shreyas-goenka
Copy link
Copy Markdown
Contributor

Do we need this for the local server? Does this speed up test runs?

All handlers that do not have a delay configured should return very fast. There seems to only be one test configured with a delay (telemetry/timeout)

@denik
Copy link
Copy Markdown
Contributor Author

denik commented May 7, 2025

Do we need this for the local server? Does this speed up test runs?

I expanded justification. This enables a few things but does not give a speed up on the current test suite.

@denik denik force-pushed the denik/acc-granular-locking branch from 9499abf to 4142907 Compare May 7, 2025 08:45
@denik denik temporarily deployed to test-trigger-is May 7, 2025 08:45 — with GitHub Actions Inactive
Comment thread acceptance/internal/prepare_server.go Outdated
Comment thread acceptance/acceptance_test.go Outdated
Comment thread acceptance/acceptance_test.go Outdated
Comment thread libs/testserver/fake_workspace.go
@denik denik force-pushed the denik/acc-granular-locking branch from 4142907 to 7796058 Compare May 7, 2025 09:29
@denik denik temporarily deployed to test-trigger-is May 7, 2025 09:29 — with GitHub Actions Inactive
@denik denik requested a review from shreyas-goenka May 7, 2025 09:29
@denik denik temporarily deployed to test-trigger-is May 7, 2025 09:37 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is May 7, 2025 09:55 — with GitHub Actions Inactive
Comment thread acceptance/internal/prepare_server.go Outdated
Comment thread libs/testserver/server.go
@denik denik temporarily deployed to test-trigger-is May 7, 2025 10:10 — with GitHub Actions Inactive
@denik denik enabled auto-merge May 7, 2025 10:11
@denik denik disabled auto-merge May 7, 2025 10:31
@denik denik merged commit 804f62a into main May 7, 2025
9 of 10 checks passed
@denik denik deleted the denik/acc-granular-locking branch May 7, 2025 10:31
denik added a commit that referenced this pull request May 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants