Sync main with short-io-sdk, add SDK test suite and CI - #4
Open
rajpootathar wants to merge 18 commits into
Open
Conversation
included section for conversion tracking with better copy
- utm_medium was appended by string concatenation keyed on contains("?"),
so a URL with a fragment and no query had the parameter placed inside the
fragment and it never reached the server. Build it with Uri instead.
- handleClick wrapped only openConnection() in Dispatchers.IO; connect()
and the header reads ran on the caller's thread. Wrap the whole body.
- trackConversion built "https:///..." with no domain and let OkHttp's
IllegalArgumentException escape. Guard and return false.
- The deprecated createShortLink overload stringified a null apiKey and
sent the literal "null" as the authorization header.
- handleClick returned the sentinel "Not Found" for a missing Location
header, which handleIntent then exposed as destinationUrl. Return null.
- Route println and printStackTrace through Log.
- Add an internal seam so the HTTP client can be replaced in tests.
Replaces the Android Studio placeholder with 23 tests across four classes. createShortLink and trackConversion are faked through an OkHttp interceptor; handleClick and handleIntent use MockWebServer, since handleClick goes through HttpURLConnection and an interceptor cannot observe it. Robolectric supplies Uri, Base64, Log and Intent. Every test was checked by mutating the source and confirming the suite fails, so the coverage is verified rather than assumed.
Runs the unit tests and lint, compiles the instrumented tests without an emulator, and rehearses the JitPack publish so a broken release surfaces in the PR rather than at tag time. JitPack publishes with -xtest -xlint, so nothing else gates a release on the suite. jitpack.yml pins JDK 17; JitPack otherwise starts on 11, finds AGP needs 17 and downloads a toolchain mid-build. Also ignores .idea and .kotlin, and untracks the .idea files already committed.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Comment |
Without initialize(), the authorization header went out empty and the caller got a 401 that gave no hint the real problem was a missing initialize(). Return a NOT_INITIALIZED error before any request. Checks the resolved key rather than isInitialized, so passing an explicit key without calling initialize() still works. Returns an error rather than throwing, matching how the function already reports failures.
There was a problem hiding this comment.
Pull request overview
This PR syncs main with the short-io-sdk branch and adds a real unit test suite plus CI workflows to validate the SDK (including deep-link handling, secure-link creation, and conversion tracking) without touching the network.
Changes:
- Updated SDK implementation (
ShortioSdk) to use a shared OkHttp client, improved click/intent handling, added secure-link support, and conversion tracking behavior. - Added Robolectric + MockWebServer based unit tests and supporting test utilities.
- Added GitHub Actions CI (unit test + lint, androidTest compilation check, and a JitPack publish rehearsal) and updated documentation.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
ShortIOSDK/src/main/java/com/github/shortiosdk/ShortIO.kt |
Major SDK logic updates: initialization, short-link creation, intent/click handling, secure URLs, conversion tracking. |
ShortIOSDK/src/main/java/com/github/shortiosdk/Model/UrlComponents.kt |
Adds destinationUrl to returned deep-link components. |
ShortIOSDK/src/main/java/com/github/shortiosdk/Model/ShortIOResult.kt |
Adjusts visibility modifiers and adds SecureResult model. |
ShortIOSDK/src/main/java/com/github/shortiosdk/Model/ShortIOParameters.kt |
Makes domain optional (String?) to support initialize-based defaults. |
ShortIOSDK/src/main/java/com/github/shortiosdk/Helpers/HelperMethods.kt |
Replaces old click helper with clid extraction + UTM stripping utilities. |
ShortIOSDK/src/main/java/com/github/shortiosdk/Constants.kt |
Renames API endpoint constant (shortenUrl → baseURL). |
ShortIOSDK/src/test/resources/robolectric.properties |
Pins Robolectric SDK level for unit tests. |
ShortIOSDK/src/test/java/com/github/shortiosdk/StubInterceptor.kt |
Adds OkHttp interceptor to stub API calls in-memory. |
ShortIOSDK/src/test/java/com/github/shortiosdk/ShortIOSdkTests.kt |
Adds unit tests for initialization, createShortLink, and trackConversion behavior. |
ShortIOSDK/src/test/java/com/github/shortiosdk/HandleIntentTests.kt |
Adds tests validating intent handling and destination URL extraction. |
ShortIOSDK/src/test/java/com/github/shortiosdk/HandleClickTests.kt |
Adds tests validating click tracking and utm_medium behavior (incl. fragments). |
ShortIOSDK/src/test/java/com/github/shortiosdk/CreateSecureTests.kt |
Adds tests validating secure-link encryption/decryption and IV reuse prevention. |
ShortIOSDK/src/test/java/com/github/shortiosdk/ExampleUnitTest.kt |
Removes placeholder test. |
ShortIOSDK/build.gradle.kts |
Enables Android resources for unit tests and adds test dependencies. |
README.md |
Updates usage/docs, adds CI badge, and documents new/updated features. |
.github/workflows/ci.yml |
Adds CI workflow: unit tests + lint, androidTest compilation, JitPack publish rehearsal. |
.github/jitpack-publish.init.gradle |
Adds init script to emulate JitPack publishing in CI. |
jitpack.yml |
Pins JDK 17 for JitPack builds. |
gradle/libs.versions.toml |
Adds versions/libs for Robolectric, MockWebServer, coroutines-test. |
.gitignore |
Simplifies IDE/Kotlin ignores. |
Suppressed comments (1)
README.md:150
- The API parameters table describes
domainas required/deprecated and "inferred from API key", but the implementation uses the domain frominitialize()whenparameters.domainis null/blank. This row should reflect the current behavior and type (String?).
| `domain` | `String` | ✅ (Deprecated) | Your Short.io domain (e.g., `example.short.gy`). ⚠️ Deprecated. No longer required — inferred from API key. May be removed in future versions. |
| `originalURL` | `String` | ✅ | The original URL to be shortened |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- trackConversion logged "createSecure failed" in its catch block. - trackConversion rethrew on transport errors, which defeats its Boolean return. Return false, matching the empty-domain guard. - handleIntent wrapped handleClick in Dispatchers.IO, which handleClick now does itself. - Correct the "SDk" typo in the not-initialized message, the Swift-style KDoc on trackConversion, and three README errors: the deprecated overload argument order, the domain parameter row, and a conversion tracking example that did not compile.
Both were broken against main by earlier releases. Cheap to restore, so restore them: - shortenUrl comes back as a deprecated alias for baseURL. - UrlComponents.destinationUrl gets a null default, so callers that constructed it before the field existed still compile. handleIntent stays suspend. It performs a network call, so the signature is correct, and a blocking overload would invite main-thread work.
Checkout leaves a repo-scoped token in .git/config for the rest of the job; nothing here pushes, so it does not need one.
The main ruleset requires a status check called test; the jobs above report under their own names, so nothing satisfied it and PRs stayed blocked.
The Uri rewrite skipped tagging whenever any utm_medium was present, so a campaign link (utm_medium=cpc) stopped being marked android. The original skipped only when already tagged android; restore that.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #3. Brings
mainup to date withshort-io-sdk, adds a test suite and CI.Work to merge
Already released as
v1.0.6–v1.0.9, never merged tomain:New here, seven defect fixes, no API changes:
utm_mediumlost on URLs with a fragment — appended by string concat keyed oncontains("?"), so…/abc#fragbecame…#frag?utm_medium=androidand never reached the server. Now usesUri.buildUpon().handleClickran network on the caller's thread —withContext(Dispatchers.IO)wrapped onlyopenConnection().trackConversionthrew instead of returning — with no domain it builthttps:///…and let OkHttp'sIllegalArgumentExceptionescape. Returnsfalsenow.createShortLinkoverload ignored the configured key — itsapiKeyparameter is declared optional (String? = null), but a null was stringified to the literal"null"and sent as the auth header instead of falling back to the key frominitialize(). Java callers hit this directly, having no default-argument resolution.handleClickreturned the sentinel"Not Found"for a missingLocation, whichhandleIntentexposed asdestinationUrl— non-null, passes a null check, not a URL. Returnsnull; the type was alreadyString?.createShortLinkcalled the API with no key — withoutinitialize()the auth header went out empty and the caller got a 401 that gave no hintinitialize()was missing. Returns aNOT_INITIALIZEDerror before any request. Checks the resolved key, notisInitialized, so passing an explicit key without initializing still works.println/printStackTrace→Log; KDoc notescreateShortLinkblocks.The combined diff mixes released and new code (
ShortIO.kt+266,README.md+102), so review by commit: nine previously released, then four new.Tests
24 tests across four classes, replacing
assertEquals(4, 2 + 2).OkHttp interceptor for
createShortLink/trackConversion; MockWebServer forhandleClick/handleIntent(it usesHttpURLConnection, which an interceptor can't see). Robolectric forUri,Base64,Log,Intent. No network.Verified by mutation: 22 mutations applied one at a time, all caught.
CI
unit-test— suite + lintinstrumentation-compile— type-checksandroidTest, no emulatorjitpack-publish— rehearses JitPack's publish commandJitPack publishes with
-xtest -xlint, so no test has ever run against a published artifact.jitpack.ymlpins JDK 17 (JitPack starts on 11, then downloads 17 mid-build).All four commits pass
testDebugUnitTestindependently, so bisect works.Follow-ups, not here
createShortLinkblocks the caller; fixing it meanssuspend, which breaks callers. Documented instead.performCreateShortLinkmutates the caller'sShortIOParameters.initializeis one-shot; a second call is silently ignored.v1.0.6–v1.0.9live onshort-io-sdk, so wherev1.1.0goes is worth a decision.v1.0.9; no change needed untilv1.1.0is tagged.