-
Notifications
You must be signed in to change notification settings - Fork 376
chore: Cleaned up and Added new tests #2541
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: ar-otel-crash-reporting
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds and refactors unit tests around the OpenTelemetry (OTel) crash reporting/uploader pipeline and related configuration/helpers, while removing an old networked integration-style test.
Changes:
- Added new unit test suites for OTel factory/open-telemetry wiring, logging helper, uploader behavior, and config builders.
- Refactored existing crash handler/reporter and attribute-field tests to reduce duplication and improve coverage.
- Removed the
CrashReportUploadTestintegration test that performed real network calls.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| OneSignalSDK/onesignal/otel/src/test/java/com/onesignal/otel/crash/OtelCrashUploaderTest.kt | New tests for uploader start behavior and constants. |
| OneSignalSDK/onesignal/otel/src/test/java/com/onesignal/otel/crash/OtelCrashReporterTest.kt | Expanded crash reporter coverage (timestamp, error paths, logging). |
| OneSignalSDK/onesignal/otel/src/test/java/com/onesignal/otel/crash/OtelCrashHandlerTest.kt | Refactored handler tests + added isOneSignalAtFault coverage. |
| OneSignalSDK/onesignal/otel/src/test/java/com/onesignal/otel/config/OtelConfigTest.kt | New tests validating config builders (resource, exporters, crash file storage). |
| OneSignalSDK/onesignal/otel/src/test/java/com/onesignal/otel/attributes/OtelFieldsTopLevelTest.kt | Refactored top-level attribute tests and added wrapper-field coverage. |
| OneSignalSDK/onesignal/otel/src/test/java/com/onesignal/otel/attributes/OtelFieldsPerEventTest.kt | Refactored per-event attribute tests and UID uniqueness checks. |
| OneSignalSDK/onesignal/otel/src/test/java/com/onesignal/otel/OtelLoggingHelperTest.kt | New tests for log-level → OTel severity mapping and emission behavior. |
| OneSignalSDK/onesignal/otel/src/test/java/com/onesignal/otel/OneSignalOpenTelemetryTest.kt | New tests for factory output types, extension helpers, and basic behavior. |
| OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/logging/otel/android/AndroidOtelLoggerTest.kt | New tests for Android OTel logger adapter basics (no-throw, interface). |
| OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapperTest.kt | Updated wrapper tests (prefs setup, multiple-start safety). |
| OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/crash/OneSignalCrashHandlerFactoryTest.kt | Updated factory tests with Robolectric config and idempotency checks. |
| OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/crash/CrashReportUploadTest.kt | Removed integration/network upload test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Thread.getDefaultUncaughtExceptionHandler() shouldBe crashHandler | ||
| verify { mockLogger.debug("OtelCrashHandler initialized") } | ||
| verify { mockLogger.info("OtelCrashHandler: Setting up uncaught exception handler...") } | ||
| verify { mockLogger.info("OtelCrashHandler: ✅ Successfully initialized and registered as default uncaught exception handler") } |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These assertions match the exact log strings (including the ✅). Log message text is typically not a stable contract and this makes the test brittle to harmless wording/formatting changes. Prefer verifying via match { it.contains(...) } (or verifying state changes, e.g., default handler set) rather than exact string equality.
| test("setAllAttributes with Map should set all string attributes") { | ||
| val mockBuilder = mockk<LogRecordBuilder>(relaxed = true) | ||
| val attributes = mapOf( | ||
| "key1" to "value1", | ||
| "key2" to "value2" | ||
| ) | ||
|
|
||
| mockBuilder.setAllAttributes(attributes) | ||
|
|
||
| // Verify the extension function was called (relaxed mock accepts all calls) | ||
| } |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test currently has no assertions/verifications, so it will pass even if setAllAttributes(Map) stops setting attributes. Consider verifying setAttribute is called for each entry (and optionally that the method returns the same builder for chaining).
| test("setAllAttributes with Attributes should handle different types") { | ||
| val mockBuilder = mockk<LogRecordBuilder>(relaxed = true) | ||
| val attributes = Attributes.builder() | ||
| .put("string.key", "string-value") | ||
| .put("long.key", 123L) | ||
| .put("double.key", 45.67) | ||
| .put("boolean.key", true) | ||
| .build() | ||
|
|
||
| mockBuilder.setAllAttributes(attributes) | ||
|
|
||
| // Verify extension function handles all types without throwing | ||
| } |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test doesn’t assert anything; it only calls setAllAttributes(Attributes). To make it meaningful, verify that setAttribute is invoked with the correct key/value types (string/long/double/boolean) and/or that unsupported types are stringified as intended.
| test("getAttributes should include all per-event fields when all values present") { | ||
| setupDefaultMocks() | ||
|
|
||
| val attributes = fields.getAttributes() | ||
|
|
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OtelFieldsPerEvent.getAttributes() sets the app state under the key "app.state" (see OtelFieldsPerEvent.kt:25), but this test later asserts "android.app.state" (in multiple places). That mismatch will fail and also encodes the wrong attribute name. Update the assertions to use "app.state".
| test("SEND_TIMEOUT_SECONDS should be 30 seconds") { | ||
| OtelCrashUploader.SEND_TIMEOUT_SECONDS shouldNotBe 0L | ||
| } |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test name says it validates SEND_TIMEOUT_SECONDS is 30 seconds, but the assertion only checks it is non-zero. This won’t catch regressions (e.g., changing to 10 or 60). Assert equality to 30L (or to the expected constant value) instead.
| // ===== OtelConfigRemoteOneSignal Tests ===== | ||
|
|
||
| test("BASE_URL should point to staging endpoint") { | ||
| OtelConfigRemoteOneSignal.SdkLoggerProviderConfig.BASE_URL shouldContain "onesignal.com" |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test description says the BASE_URL should point to the staging endpoint, but the assertions only check for onesignal.com and /sdk/otel. Since the current value is https://api.staging.onesignal.com/sdk/otel, consider asserting the actual expected host/prefix (e.g., contains api.staging.onesignal.com) or rename the test to match what is being asserted.
| OtelConfigRemoteOneSignal.SdkLoggerProviderConfig.BASE_URL shouldContain "onesignal.com" | |
| OtelConfigRemoteOneSignal.SdkLoggerProviderConfig.BASE_URL shouldContain "api.staging.onesignal.com" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ore/src/test/java/com/onesignal/debug/internal/logging/otel/android/AndroidOtelLoggerTest.kt
Show resolved
Hide resolved
...ignalSDK/onesignal/otel/src/main/java/com/onesignal/otel/config/OtelConfigRemoteOneSignal.kt
Show resolved
Hide resolved
OneSignalSDK/onesignal/otel/src/test/java/com/onesignal/otel/crash/OtelCrashUploaderTest.kt
Show resolved
Hide resolved
OneSignalSDK/onesignal/otel/src/test/java/com/onesignal/otel/crash/OtelCrashUploaderTest.kt
Show resolved
Hide resolved
OneSignalSDK/onesignal/otel/src/test/java/com/onesignal/otel/OtelLoggingHelperTest.kt
Show resolved
Hide resolved
OneSignalSDK/onesignal/otel/src/test/java/com/onesignal/otel/crash/OtelCrashReporterTest.kt
Outdated
Show resolved
Hide resolved
...l/core/src/test/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapperTest.kt
Show resolved
Hide resolved
...al/core/src/test/java/com/onesignal/debug/internal/crash/OneSignalCrashHandlerFactoryTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fun setupDefaultMocks( | ||
| remoteLogLevel: String? = "ERROR", | ||
| crashStoragePath: String = "/test/crash/path", | ||
| minFileAgeForReadMillis: Long = 100L | ||
| ) { | ||
| every { mockPlatformProvider.remoteLogLevel } returns remoteLogLevel | ||
| every { mockPlatformProvider.crashStoragePath } returns crashStoragePath | ||
| every { mockPlatformProvider.minFileAgeForReadMillis } returns minFileAgeForReadMillis | ||
| every { mockRemoteTelemetry.logExporter } returns mockExporter | ||
| every { mockExporter.export(any()) } returns CompletableResultCode.ofSuccess() |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests call uploader.start(), which creates a FileLogRecordStorage rooted at "/test/crash/path". On many CI environments this absolute path won't exist or won't be writable, causing test failures/flakiness. Use a per-test temp directory (e.g., under java.io.tmpdir), ensure it exists, and clean it up after the test.
OneSignalSDK/onesignal/otel/src/test/java/com/onesignal/otel/crash/OtelCrashUploaderTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
OneSignalSDK/onesignal/otel/src/test/java/com/onesignal/otel/crash/OtelCrashHandlerTest.kt
Show resolved
Hide resolved
📊 Diff Coverage ReportDiff Coverage ReportThreshold: 80% Changed Files Coverage
Overall Coverage1075/4025 lines covered (26.7%) ❌ Coverage Check FailedFiles below 80% threshold:
|
Description
One Line Summary
Adding unit tests and refactoring existing ones
Also updated the production url
Details
Motivation
We added new functionality and want to make sure we have enough coverage.
Scope
Unit tests only
Testing
Unit testing
Added them
Manual testing
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is