Skip to content

feat: add test - #169

Open
Balamurugan-100 wants to merge 1 commit into
mainfrom
add_test
Open

feat: add test#169
Balamurugan-100 wants to merge 1 commit into
mainfrom
add_test

Conversation

@Balamurugan-100

Copy link
Copy Markdown
Member

No description provided.

@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Claude finished @Balamurugan-100's task in 3m 56s —— View job


Claude's Review

Tasks

  • Gather context (read CLAUDE.md — none present; diff vs origin/main)
  • Review code quality (force-unwraps, weak self, TPStreamPlayerError usage)
  • Review resource lifecycle (KVO/time observers, deinit cleanup, Realm threading)
  • Review public API / backward compatibility (#if SPM/CocoaPods guards, TPStreamPlayerConfiguration)
  • Review security (tokens in logs/Sentry, DRM key exposure)
  • Post review

This PR is overwhelmingly a test-suite addition (44 files, +6220/-44): 20+ new unit/integration test files, mock network/URLProtocol infrastructure for SampleAppIntegrationTests, a TEST_SUITE_GUIDE.md, an expanded two-job (pr-gate/release-gate) CI workflow, and a handful of small production-code tweaks to make the SDK testable. No public API surface changed in a breaking way, and I found no token/DRM-key leakage into logs or Sentry.

Critical — build-breaking duplicate Xcode project entries
iOSPlayerSDK.xcodeproj/project.pbxproj: 5 test files (TimeStringTests.swift, StreamsAPIParserTests.swift, TPStreamsDownloadManagerUnitTests.swift, TPAVPlayerUnitTests.swift, EncryptionKeyDelegateTests.swift) each have two PBXFileReference/PBXBuildFile entries pointing at the same on-disk file, and both are listed in the iOSPlayerSDKTests target's single Sources build phase. Compiling the same file twice into one target will fail xcodebuild test in CI (see inline comment for exact line numbers/IDs). This should be fixed before merge — I'd expect the pr-gate job to currently fail.

Other findings (inline comments posted)

  • Source/Network/BaseAPI.swift:14 — new static var session: Session = AF test seam looks unused; SampleAppIntegrationTests mocks networking via global URLProtocol.registerClass instead, so this is dead code (or a wiring gap).
  • Source/TPStreamsSDK.swift:40realmConfig changed from let to var to support configureInMemoryRealm in tests. Reasonable (internal-only, no public API break), just flagging it as a shared mutable global that could race under parallel test execution.
  • .github/workflows/ios_test.yml:64release-gate's "Full Test Suite" step is identical to pr-gate's (same simulator destination), so it doesn't actually add device-dependent DRM/Keychain/download coverage as the comment/report claims — those tests self-skip on simulator per TEST_SUITE_GUIDE.md.

Nice catch

  • Source/Utils/Time.swift:11guard seconds.isFinite || !seconds.isNaNguard seconds.isFinite, !seconds.isNaN fixes a real bug: the old || condition let .infinity slip past the guard (only NaN was actually caught). Good fix.

I was unable to run xcodebuild/the test suite myself (no macOS/Xcode toolchain available in this environment), so I could not execute the new tests to confirm they pass beyond the static project-file analysis above.

Comment on lines +996 to +1028
F85EAAE2DD49D40B792DF73E /* TimeStringTests.swift in Sources */,
2803F4BE687154EEFC062D0D /* StreamsAPIParserTests.swift in Sources */,
FE102F814273D8F204B0132B /* TPStreamsDownloadManagerUnitTests.swift in Sources */,
B8BEE38F734E695468B75F1B /* TPAVPlayerUnitTests.swift in Sources */,
AEE26E08B64276C686D9C418 /* EncryptionKeyDelegateTests.swift in Sources */,
0802945E0BC0643DEC4ABF52 /* APIURLConstructionTests.swift in Sources */,
7D4E463916DA580ABF2E0382 /* AnyRealmValueTests.swift in Sources */,
1D215431F5997E3DC8EFCB46 /* AssetModelTests.swift in Sources */,
DBA54790141B57ABF9636B08 /* ClientMisuseIntegrationTests.swift in Sources */,
420A1D41A37190EB220D4810 /* ContentKeyDelegateTests.swift in Sources */,
75D8EF3B9D673B5FE1B175FC /* DatabaseTests.swift in Sources */,
6D714567731E06140C7CE1D9 /* EncryptionKeyDelegateTests.swift in Sources */,
735DEB1ED1B4F0FDA722323E /* KeychainUtilTests.swift in Sources */,
438C9125B6E6822A4E07287C /* M3U8ParserTests.swift in Sources */,
3CB3F1C12A1FA29E369F6C08 /* ObjectManagerTest.swift in Sources */,
9F922E608D3CBC2C0C74E7DE /* PlaybackSpeedTests.swift in Sources */,
9167AE7245B87C1D33C7E56F /* PlayerLifecycleIntegrationTests.swift in Sources */,
749C64EA81DAA36F062239E2 /* PlayerStateIntegrationTests.swift in Sources */,
9A3F2382D437A7DD4E63B30D /* ResourceLoaderDelegateTests.swift in Sources */,
0C217359A39E78B4EED581BB /* SDKInitIntegrationTests.swift in Sources */,
B6BD19B94C1C570A26C8EFDC /* SentryCaptureTests.swift in Sources */,
1096D9CA5362CAF1E0262EBB /* StreamsAPIParserTests.swift in Sources */,
4909B07A40ACFAE2C37222D4 /* SubtitleTrackTests.swift in Sources */,
6D20613AECD12AE715B44E67 /* TPAVPlayerUnitTests.swift in Sources */,
BFB66224412D53481ECCDE87 /* TPStreamPlayerConfigurationTests.swift in Sources */,
48297203731E99017FD7DC1D /* TPStreamPlayerErrorTests.swift in Sources */,
07BB2F8F1C2AD0B88F9E785D /* TPStreamPlayerViewModelTests.swift in Sources */,
366F470EC85C4E707E97E524 /* TPStreamsDownloadManagerTests.swift in Sources */,
0F03D58438414C0696B3EE26 /* TPStreamsDownloadManagerUnitTests.swift in Sources */,
267D6A67AD61D5287C00EE0A /* TPStreamsSDKTests.swift in Sources */,
889B9EE6CB490E082DDEE50E /* TestHelpers.swift in Sources */,
A05535A5B53244D2E1B0B655 /* TestpressAPIParserTests.swift in Sources */,
D73F86A141C44619EFDA935B /* TimeStringTests.swift in Sources */,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Critical: duplicate PBXBuildFile/PBXFileReference entries — this will break the build.

Five test files are each referenced twice by separate PBXFileReferences and compiled twice into the same iOSPlayerSDKTests target's Sources build phase (this phase, 8EDE99AA2A2643B000E43EA9):

  • TimeStringTests.swift — build files D73F86A1... (fileRef D59FE315...) and F85EAAE2... (fileRef D9184752...), both listed at lines 996 & 1028
  • StreamsAPIParserTests.swift1096D9CA.../098425DC... and 2803F4BE.../102E875A..., lines 997 & 1017
  • TPStreamsDownloadManagerUnitTests.swift0F03D584.../9170F2A5... and FE102F81.../B91AE02F..., lines 998 & 1024
  • TPAVPlayerUnitTests.swift6D20613A.../26838F8E... and B8BEE38F.../DF966C95..., lines 999 & 1019
  • EncryptionKeyDelegateTests.swift6D714567.../373AC5A0... and AEE26E08.../50AA8F98..., lines 1000 & 1007

There is only one physical copy of each file on disk (in Tests/), so both PBXFileReferences point at the same file. Compiling the same Swift file twice into one target causes Xcode to fail with a "Multiple commands produce ...” / duplicate-symbol build error, which will fail the CI pr-gate/release-gate jobs entirely.

Fix: remove one PBXFileReference + matching PBXBuildFile entry for each of the 5 files (and its single duplicate line from this Sources phase), keeping only one.

Fix this →

class BaseAPI {
/// The Alamofire Session used for all requests.
/// Tests can override this to inject a mock session with custom URLProtocol.
static var session: Session = AF

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This new static var session: Session = AF seam appears unused — none of the new test files (Tests/, SampleAppIntegrationTests/) actually override BaseAPI.session. SampleAppIntegrationTests mocks the network via global URLProtocol.registerClass(MockURLProtocol.self) instead (see SampleAppIntegrationTests/MockURLProtocol.swift), which already intercepts AF's default session, making this injection point dead code. Either wire a test into it or drop it to avoid an unused, mutable global Session in production code (also worth a [weak self]-style guard/reset note: a global mutable static var used only by some future test could leak state across test runs if ever adopted without a teardown).

Comment thread Source/TPStreamsSDK.swift

public class TPStreamsSDK {
internal static let realmConfig: Realm.Configuration = buildRealmConfig()
internal static var realmConfig: Realm.Configuration = buildRealmConfig()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Changing realmConfig from let to var so Tests/TestHelpers.swift:configureInMemoryRealm can swap in an in-memory config is reasonable for testability, but note it's a process-wide mutable global now (internal, not public, so no API-compat concern). If the test target ever enables parallel test execution, concurrent test classes mutating this shared static could race/flake. Not a blocker, just worth a // test-only mutation point note or restricting the setter, since ObjectManager.swift:12 reads it on every Realm access.


# ─────────────── JOB 2: Release Gate ───────────────
# Runs only when code lands on main. Adds:
# - Full test suite (including device-dependent tests)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The release-gate job's "Full Test Suite" step re-runs the identical xcodebuild test command as pr-gate — same scheme, same platform=iOS Simulator destination — so it does not actually add device-dependent coverage despite the comment "Adds: Full test suite (including device-dependent tests)". Per TEST_SUITE_GUIDE.md, DRM/AES/Keychain/offline-download tests are runtime-skipped on simulator via #if targetEnvironment(simulator)/isSimulator guards, so they'll silently no-op here too. The closing "Report" step then prints "✅ Unit tests: 384+ passing" / "SDK is ready for client handoff", which overstates coverage since the device-only tests never ran. Consider either running this job on a real device runner (self-hosted/cloud) or rewording the job description and report to make clear device-only tests are still not covered by CI.

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.

1 participant