test: add unit test suite and pull_request CI

[claude]

Requested by mwfarb · Slack thread

WIP / draft.

Before

There are no C# tests anywhere in the repository, and nothing at all is triggered by pull_request — grep -rn pull_request .github/ returns nothing on main. The only automated checks are the two native-plugin builds (pinned to feature branches) and release-please. RELEASE.md's prerequisites are entirely manual: "ensure the project compiles" and "hit Play and verify".

Tests/ exists but holds two shell scripts and eight JSON scene dumps — no test code.

After

Two suites, split by what they need to run rather than by what they cover, and a pull_request workflow that gates on the half which needs no licence.

Suite	Location	Needs	In CI
Pure .NET	Tests~/DotNet/ArenaUnity.PureTests	.NET 8 SDK	Always — this is the gate
Unity EditMode	Tests/Editor	Unity 6 Editor	Only with licence secrets

What the .NET job gates today: 273 tests, ~150 ms, no secrets, so it runs identically on fork pull requests. It covers all 29 ArenaTopics getters as exact strings plus the positional invariants the inbound router depends on, ArenaCssColors.Normalize and its 148-entry table, the colour JSON converters, the full legacy-tolerance matrix of the boolean-object and string-object converters, and all 19 environment presets.

The Unity EditMode job stays skipped until licence secrets exist. A ${{ secrets.* }} expression is not evaluated in a job-level if:, so a small check-unity-license job probes for the secrets and publishes a job output that unity-editmode-tests gates on with needs:/if:. A skipped job does not fail the pull request, and secrets are never exposed to fork pull requests, so fork contributions skip it by design. To enable it, add under Settings > Secrets and variables > Actions either UNITY_LICENSE, or UNITY_EMAIL + UNITY_PASSWORD + UNITY_SERIAL.

How

The .NET project never copies package source — it links the real files out of Runtime/ as MSBuild Compile items, so the tests always compile the shipping code. Only the five sources verified free of UnityEngine are linked in. No UnityEngine shim, stub, or fake exists anywhere in this branch; a test that exercised a reimplementation of Vector3 would prove nothing about the real one, so anything needing Unity went to the EditMode suite instead. ArenaVectorJsonConverter was a candidate but its ArenaVector3Json / ArenaVector2Json types use [Tooltip] and Debug.LogWarning, so it is covered from the Unity side.

Tests~/ uses the same ~ convention as Samples~/ and Documentation~/, so UPM and Unity ignore it and nothing under it needs a .meta. Tests/Editor is imported, so every file there has one. The test assembly mirrors the Editor/VersionCheck isolation pattern: Editor-only, autoReferenced: false, gated on UNITY_INCLUDE_TESTS.

Because arena-unity is a UPM package with no Assets/ or ProjectSettings/, the Unity job creates a throwaway project, adds the checked-out package from disk, and sets "testables": ["io.conix.arena.unity"]. com.unity.test-framework is pinned to 1.4.6 per the pin-all-versions rule; it brings com.unity.ext.nunit (NUnit 3.5), so EditMode assertions stay inside the NUnit 3.0 API surface. Every action is SHA-pinned with the mandatory inline # vX.Y.Z comment.

com.unity.test-framework is not added to the package's dependencies by this branch. It is needed to run the tests, not to use the library, so forcing it on every consumer would be wrong. main never listed it: an earlier commit here (dfff1fe) added it to package.json and a later one (8f0bcec) took it back out, so package.json is unchanged relative to main and is not among this branch's changed files. The project that runs the tests declares the dependency itself, exactly pinned, next to its testables entry. The CI harness manifest above already did exactly that, so the EditMode job is unaffected, and Documentation~/Testing.md and CONTRIBUTING.md now tell a developer to do the same locally. Tests/Editor's asmdef is unchanged. What keeps that assembly out of a consumer's project is the testables gate, not its UNITY_INCLUDE_TESTS define constraint: UNITY_INCLUDE_TESTS is defined whenever the Editor compiles, so the constraint is satisfied there. What the constraint does is mark the assembly as a test assembly, which is both what subjects it to the testables check (CustomScriptAssembly.IsCompatibleWith returns false for a test assembly whose package metadata is not IsTestable) and what drops it from player builds, where the symbol is not defined. So a project that adds the package without naming it in testables never compiles that assembly, and its UnityEngine.TestRunner and nunit.framework.dll references are never resolved there.

New doc at Documentation~/Testing.md, linked from the README reference list, the AGENTS.md index, and CONTRIBUTING.md. RELEASE.md's prerequisites now start with running the tests.

Tests I have not run myself

The EditMode suite. There is no Unity Editor in the environment that produced this branch, so those ~1,900 lines are unexecuted. To keep that honest:

• Every file was compiled against the real NUnit and Newtonsoft.Json assemblies with only UnityEngine, UnityEditor and conix.arena.unity left unresolved: no syntax errors, no NUnit or Newtonsoft API misuse.
• Assertions that could not be derived with certainty from the source were left out rather than guessed — arbitrary-rotation round trips through eulerAngles, ColorUtility internal representation, Shader.Find paths, ParametricPlaneRandom, the internal mesh builders, and anything touching AR Foundation.
• The most likely first-run failure is LogAssert.NoUnexpectedReceived() in the scene-corpus tests, which would be genuine signal: it would mean a real persisted scene trips an [OnError] handler.

The .NET suite was mutation-tested — 21 mutations across all five fixtures (a changed topic string, a deleted getter, a dropped dictionary entry, inverted branches, a broken comparer, corrupted preset values), each confirmed to fail a specific named test, each reverted, with git diff on Runtime/ empty afterwards. No mutation survived.

Behaviours pinned as currently-buggy

These tests assert behaviour that is wrong, so a known fault cannot change silently. Each carries a // PINS CURRENT BEHAVIOUR (bug) comment naming file:line, the correct behaviour, and the assertion to flip. Fixing one of these means flipping its assertion, not deleting the test.

Filed separately (this branch pins them, it does not fix them):

• Base64UrlDecode never maps - to + or _ to /, so a JWT payload segment in the base64url alphabet throws FormatException; all three callers feed it a JWT. Pinned in the EditMode suite, with the post-fix assertion already written and [Ignore]d. Refs fix: Base64UrlDecode does not decode base64url, breaking JWT parsing #180
• JavaScript regex literal defeats filename sanitization in ArenaClientScene.Export.cs:158. Not pinned by any test — it sits inside a public IEnumerator instance method that authenticates and uploads; it needs the sanitization extracted to a static helper first. Refs fix: filename sanitization in UploadStoreFile uses a JavaScript regex literal #181
• Tick count stored in a PlayerPrefs float in Editor/ArenaVersion.cs:147. Not pinned by any test — ArenaVersion is an internal Editor class whose throttle sits in a static constructor and a coroutine that calls the GitHub API. Refs fix: version-check throttle loses precision storing DateTime ticks in a float #182

Found while writing these tests, not yet filed:

• ArenaColorArrayJsonConverter.WriteJson (ArenaColorJsonConverter.cs:274) throws on a null array, unlike its scalar sibling. Newtonsoft's null-member short-circuit hides it on the common path.
• ArenaBooleanObjectJsonConverter.WriteJson (ArenaBooleanObjectJsonConverter.cs:58) throws on null, although ReadJson returns null for both JSON null and boolean false.
• ArenaStringObjectJsonConverter.cs:36 formats numbers with CurrentCulture, so under a comma-decimal locale an animation to / from value goes out as 1,5 and the A-Frame client cannot parse it. The same line emits .NET True / False casing for booleans instead of JavaScript's lowercase. Pinned with the culture set explicitly so the test does not depend on runner locale.
• SphereBuilder.cs:54 over-allocates its index buffer and leaves a tail of degenerate zero-area triangles — 36 at the default 24x16 segmentation.
• new ArenaTopics() zero-initializes the struct rather than running the all-optional-argument constructor, so every segment property is null, not empty string. The topic strings still come out well-shaped because interpolation renders null as empty, but topics.nameSpace.Length throws.
• PlaneBuilder.Build divides by segments - 1 after clamping to 1, so a segment count below 1 yields NaN vertices. Not tested, to avoid a NaN-producing mesh in the suite.
• meeting.json uses object_type: "prism", which is handled nowhere in the package.

---

Generated by Claude Code