Skip to content

Comments

test: finished testing 30 players joining#272

Merged
AlexanderHeffernan merged 2 commits intomainfrom
271-30-player-intergration-test-backend
Oct 5, 2025
Merged

test: finished testing 30 players joining#272
AlexanderHeffernan merged 2 commits intomainfrom
271-30-player-intergration-test-backend

Conversation

@Ming-Bao
Copy link
Contributor

@Ming-Bao Ming-Bao commented Oct 5, 2025

No description provided.

@Ming-Bao Ming-Bao linked an issue Oct 5, 2025 that may be closed by this pull request
@AlexanderHeffernan AlexanderHeffernan merged commit 2a8792a into main Oct 5, 2025
1 check passed
@Ming-Bao Ming-Bao deleted the 271-30-player-intergration-test-backend branch October 5, 2025 23:17
Copy link
Contributor

@AlexanderHeffernan AlexanderHeffernan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ Pull Request Review

Status: Not ready—address blockers
Blockers: 2
Suggestions: 2
Reviewed: December 14, 2025

The PR introduces a load test for player joining, which is a good initiative. However, the test currently lacks assertions and proper asynchronous handling, meaning it doesn't effectively verify the functionality.

Strengths

🟡 Suggestions

  • The loop i <= 30 creates 31 connections (0 to 30). Confirm if this off-by-one behavior is intentional or if it should be i < 30. Ref: backend/test/api.test.ts:224.
  • Avoid hardcoding ws://localhost:3000. Use a dynamic port/URL from the test environment configuration to ensure CI reliability. Ref: backend/test/api.test.ts:226.

🔴 Blockers

  • [Critical] No Assertions: The test merely initiates connections but verifies nothing. You must assert that the lobby player count reached 30 or that the connections were successful. Ref: backend/test/api.test.ts:223.
  • [Critical] Async Handling: new WebSocket(...) does not await the connection. The test will likely exit before connections are established. You need to wait for the open event or use a promisified client to ensure connections are active before asserting.

This review was generated by Pull Request Reviewer, an AI agent operating on the Autohive platform.

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.

30 player intergration test backend

2 participants