Support Unity 6.5/6.6 (EntityId migration) + timezone regression tests - #488
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new tests are missing the LootLockerCI category and include clamping assertions that will fail on older supported Unity versions where clamping is not compiled in.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Updates the SDK to compile on Unity 6.5/6.6 by replacing deprecated Object.GetInstanceID() usage in LootLockerLifecycleManager, and adds regression tests to lock in the exact IANA timezone strings emitted by LootLockerTimezoneConverter.
Changes:
- Store lifecycle-manager identity using
EntityId+GetEntityId()on Unity 6.5+ while keepingGetInstanceID()on older editors viaUNITY_6000_5_OR_NEWER. - Add
TimezoneConverterTeststo validate casing/sign conventions and conversion/fallback behavior for IANA timezone strings. - Add the corresponding Unity
.metafile for the new test.
| File | Description |
|---|---|
| Runtime/Client/LootLockerLifecycleManager.cs | Uses EntityId/GetEntityId() behind UNITY_6000_5_OR_NEWER to fix Unity 6.5+ compile errors while preserving older-editor behavior. |
| Tests/LootLockerTests/PlayMode/TimezoneConverterTests.cs | Adds regression coverage for timezone conversion outputs (IANA casing, sign convention, Windows/IANA passthrough, fallbacks). |
| Tests/LootLockerTests/PlayMode/TimezoneConverterTests.cs.meta | Unity asset metadata for the new test file. |
Files not reviewed (1)
- Tests/LootLockerTests/PlayMode/TimezoneConverterTests.cs.meta: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Out-of-range offsets are clamped | ||
| Assert.AreEqual("Etc/GMT+12", LootLockerTimezoneConverter.convertUTCOffsetToIanaTzString(-999)); | ||
| Assert.AreEqual("Etc/GMT-14", LootLockerTimezoneConverter.convertUTCOffsetToIanaTzString(999)); |
There was a problem hiding this comment.
Good catch — fixed in 21f2e81. The out-of-range clamping assertions are now wrapped in #if UNITY_2021_1_OR_NEWER to match the converter's own clamping guard, so they no longer assert on older supported editors (2019.2/2020) where clamping doesn't occur.
| /// </summary> | ||
| public class TimezoneConverterTests | ||
| { | ||
| [Test, Category("LootLocker"), Category("LootLockerCIFast")] |
There was a problem hiding this comment.
Fixed in 21f2e81 — all three tests now include the LootLockerCI category alongside LootLocker and LootLockerCIFast, matching JsonTests.cs. They'll run both on PRs to dev (LootLockerCIFast) and PRs to main (LootLockerCI).
|
Note on the failing integration tests jobs: these fail identically on other PRs (eg #486) — the devenv |
Unity 6.5+ obsoletes Object.GetInstanceID with a compile error (CS0619), which broke the SDK on Unity 6.5/6.6. Guard the migration behind UNITY_6000_5_OR_NEWER so older supported editors keep working. Also add tests locking in the IANA timezone casing/sign convention produced by LootLockerTimezoneConverter (Etc/GMT, not ETC/GMT). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Tag tests with the LootLockerCI category so they run on PRs targeting main. - The out-of-range clamping assertions only hold from UNITY_2021_1_OR_NEWER, since the converter only clamps under that define; guard them accordingly.
21f2e81 to
0a1a363
Compare
Summary
Object.GetInstanceID()(error CS0619, "Use GetEntityId instead"), which broke the SDK on Unity 6.5/6.6.LootLockerLifecycleManagernow stores the instance id asEntityIdand usesGetEntityId()when compiling against Unity 6.5+, and keepsGetInstanceID()/inton older versions (guarded byUNITY_6000_5_OR_NEWER), so the minimum supported editor (2019.2) is unchanged.TimezoneConverterTestslocking in the exact IANA timezone strings (Etc/GMT+X,Etc/GMT-X,Etc/UTC) produced byLootLockerTimezoneConverter. This guards against the casing/sign class of bug reported by a customer against the Unreal sample (ETC/GMTis invalid; must beEtc/GMT).Testing
LootLockerLifecycleManager.cs(118,31)is gone.TimezoneConverterTests(LootLockerCIFast).Ref: lootlocker/index#1722