Skip to content
This repository was archived by the owner on Aug 18, 2026. It is now read-only.

Add unit tests to increase coverage and fix race condition in GameServersQueue - #505

Merged
Dimitris Gkanatsios (dgkanatsios) merged 7 commits into
mainfrom
copilot/add-unit-tests-for-coverage
Mar 24, 2026
Merged

Dimitris Gkanatsios (dgkanatsios) merged 7 commits into
mainfrom
copilot/add-unit-tests-for-coverage

Conversation

Copilot AI commented Mar 24, 2026 •

Copy link
Copy Markdown
Contributor

53 new unit tests across 5 packages, targeting the largest coverage gaps.

cmd/nodeagent — 22 tests (new file: nodeagentmanager_unit_test.go)

  • heartbeatHandler: non-existent GS, logEveryHeartbeat, ignoreHealthFromHeartbeat, Active/StandingBy operation responses
  • updateHealthAndStateIfNeeded: no-op on unchanged state, valid/invalid transitions, crash recovery (empty→Active)
  • updateConnectedPlayersIfNeeded: skip when not Active, skip when count unchanged, count changes, zero players
  • gameServerCreatedOrUpdated: new server added to map, non-Active skip, Active+Healthy session details propagation
  • gameServerDeleted: map removal, no-op for missing server, DeletedFinalStateUnknown handling
  • HeartbeatTimeChecker: recent heartbeat no-mark, already-unhealthy no-mark, no double-mark

cmd/initcontainer — 5 tests

  • createGsdkFolders (create + idempotent), multi-port parsing, nested path failure, GsdkConfig JSON key validation

cmd/latencyserver — 3 test specs

  • createServer with port 0, getResponse edge cases via DescribeTable, serverLoop end-to-end

cmd/standby-forecaster — 5 tests

  • Decreasing regression prediction, increasing forecast series, K8s config loading, metric conversion config, flag overrides

pkg/operator/controllers — 18 tests

  • IsNodeReadyAndSchedulable: ready, unschedulable, no condition, Ready=False, multiple conditions
  • randString: length and character set
  • getValueByState: all state→int mappings
  • ByState: sort ordering verification
  • NewPodForGameServer: Linux, Windows, label/annotation preservation
  • NewGameServerForGameServerBuild: labels, ownerRef, hostPort assignment

Bug fix: race condition in GameServersQueue.PushToQueue

Fixed a pre-existing race condition in PushToQueue that caused the "should work with multiple simultaneous requests" test to be flaky. The old code used a read-lock to check if a build's queue existed, released it, then acquired a write-lock — a concurrent PopFromQueue could delete the build entry in between, causing items to be lost. Fixed by holding a single write-lock for the entire operation.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 5 commits March 24, 2026 02:52
Add testify-based unit tests in nodeagentmanager_unit_test.go covering:
- heartbeatHandler: non-existent GS, logEveryHeartbeat, ignoreHealthFromHeartbeat,
  Active+StandingBy=>Active operation, Active+Active=>Continue
- updateHealthAndStateIfNeeded: no-change, valid transitions, invalid transitions,
  Active recovery after NodeAgent crash
- updateConnectedPlayersIfNeeded: not-active skip, same-count skip, count changes,
  zero players
- gameServerCreatedOrUpdated: new server, non-active skip, Active+Healthy session
  details, existing server update
- gameServerDeleted: removal, non-existent no-op, DeletedFinalStateUnknown wrapping
- HeartbeatTimeChecker: recent heartbeat, already-unhealthy, already-marked

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add 5 new test functions covering previously untested code paths:
- TestCreateGsdkFolders: tests server log directory creation
- TestParseBuildMetadataMultipleSeparators: edge cases with separators
- TestParsePortsMultiplePorts: tests with 3+ ports
- TestCreateFolderIfNotExistsNestedPath: verifies os.Mkdir behavior
- TestGsdkConfigJsonStructure: validates JSON serialization keys

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- latencyserver: TestCreateServerInvalidPort, TestGetResponseEdgeCases
  (table-driven via DescribeTable), TestServerLoopValidMessage
- standby-forecaster: TestGetLinearRegressionPredictionDecreasing,
  TestGetForecastedServerCountIncreasing, TestLoadConfigK8sSettings,
  TestLoadConfigMetricConversion, TestConfigRegisterFlagsK8sOverrides

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add new test cases for:
- IsNodeReadyAndSchedulable (5 cases: ready/schedulable, unschedulable,
  no ready condition, ready=false, multiple conditions)
- randString (correct length, character set validation)
- getValueByState (empty, initializing, standingBy, active, crashed)
- ByState sort interface (ordering verification)
- NewPodForGameServer (Linux, Windows, annotations/labels preserved)
- NewGameServerForGameServerBuild (labels, ownerRef, hostPorts)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: dgkanatsios <8256138+dgkanatsios@users.noreply.github.com>
Agent-Logs-Url: https://github.com/PlayFab/thundernetes/sessions/9b87433c-2cb0-4781-9f8c-c4736f12767b
@dgkanatsios
Dimitris Gkanatsios (dgkanatsios) marked this pull request as ready for review March 24, 2026 03:13
@dgkanatsios

Copy link
Copy Markdown
Collaborator

Copilot unit tests failed

The previous implementation used a read-lock to check existence, released it,
then acquired a write-lock. Between these operations a concurrent PopFromQueue
could delete the build entry, causing items to be lost.

Fix: hold a single write-lock for the entire PushToQueue operation.

Co-authored-by: dgkanatsios <8256138+dgkanatsios@users.noreply.github.com>
Agent-Logs-Url: https://github.com/PlayFab/thundernetes/sessions/53b53407-f735-4dbd-844c-6a664eb45be9

Copilot AI commented Mar 24, 2026

Copy link
Copy Markdown
Contributor Author

Copilot unit tests failed

The failing test (gameserverqueue tests - should work with multiple simultaneous requests) was caused by a pre-existing race condition in GameServersQueue.PushToQueue, not by the new unit tests. The old code used a read-lock to check build existence, released it, then acquired a write-lock — a concurrent PopFromQueue could delete the build entry in between, causing items to be lost.

Fixed in 569f8cd by holding a single write-lock for the entire PushToQueue operation.

Copilot AI changed the title Add unit tests to increase coverage across all packages Add unit tests to increase coverage and fix race condition in GameServersQueue Mar 24, 2026
@dgkanatsios
Dimitris Gkanatsios (dgkanatsios) merged commit dc23dd7 into main Mar 24, 2026
7 checks passed
@dgkanatsios
Dimitris Gkanatsios (dgkanatsios) deleted the copilot/add-unit-tests-for-coverage branch March 24, 2026 03:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants