From 012fe09bc940154075d463cb08b22a302900ec71 Mon Sep 17 00:00:00 2001 From: Fiona Date: Wed, 2 Sep 2026 19:33:35 -0700 Subject: [PATCH 1/2] ci: run the library unit tests and the formatter on every pull request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The only workflow reaching a pull request was CodeQL, so nothing ran the tests or the formatter before a change landed on publish. Both gates existed already — as gradle tasks and in local_ci.sh — but only for whoever remembered to run them. Unit tests go through the project's own unitTestDebug aggregation, which covers the dd-sdk-android-* modules and nothing else. That deliberately leaves out the sample applications: assembling them with their Us1-Us5 flavors is what once pushed the publish job past two hours, and their tests are not what a library change needs to be judged on. KtLint judges the files the pull request changed rather than the tree. This is a fork that tracks upstream, and the inherited tree carries around seventy findings, nearly all of them in files we do not write — the sample applications, the no-op modules, some build scripts. Reformatting those would buy a conflict in every one of them at the next upstream merge, paid forever, for code that is not ours. Judging the diff keeps what we do write clean without taking on that bill. Detekt is deliberately absent. Its shared configuration lives in a repository this project does not have access to, and the custom rules need a full assemble first for type resolution — a far heavier job than either of these, and worth deciding on separately rather than smuggling in behind them. The version of ktlint is pinned to the one local_ci.sh installs, so the answer here is the answer a developer gets on their own machine. --- .github/workflows/ci.yml | 134 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 .github/workflows/ci.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000000..87739f4174 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,134 @@ +name: CI + +# The gate that was missing: until now the only workflow running on a pull +# request was CodeQL, so nothing ran the unit tests or the formatter before a +# change reached `publish`. Everything here is deliberately reachable without +# any credential, so a fork or a first-time contributor gets the same verdict. +on: + pull_request: + branches: ["publish"] + push: + branches: ["publish"] + +# A run for a superseded commit tells nobody anything, and these jobs are not +# cheap in wall-clock. Only the newest commit on a branch keeps its run. +concurrency: + group: ci-${{ github.ref }} + cancel-in-progress: true + +jobs: + unit-tests: + name: Unit tests + runs-on: ubuntu-latest + # Generous on purpose: forty module test tasks on a cold Gradle cache take + # far longer than a warm local run, and killing a legitimately slow build + # teaches nothing. A hang only wastes minutes, which are free on this + # public repository. + timeout-minutes: 90 + + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Set up JDK 17 + uses: actions/setup-java@v4 + with: + java-version: "17" + distribution: "temurin" + + - name: Setup Android SDK + uses: android-actions/setup-android@v3 + + - name: Cache Gradle packages + uses: actions/cache@v4 + with: + path: | + ~/.gradle/caches + ~/.gradle/wrapper + key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle*', '**/gradle-wrapper.properties') }} + restore-keys: | + ${{ runner.os }}-gradle- + + - name: Grant execute permission for gradlew + run: chmod +x gradlew + + # `unitTestDebug` is the project's own aggregation task, and it covers + # exactly the modules named `dd-sdk-android-*` — the SDK libraries. The + # sample applications are not among them, which is what keeps this job + # from repeating the mistake the publish workflow documents: assembling + # the samples with their Us1-Us5 flavors once pushed that job past two + # hours. + # + # No `clean`. The runner is ephemeral, so there is nothing stale to + # remove, and cleaning in the same invocation races with Kover. + - name: Run the library unit tests + run: ./gradlew unitTestDebug --stacktrace + + # Only on failure: the reports are what turns "some task failed" into a + # named test, and on a green run nobody opens them. + - name: Upload the test reports + if: failure() + uses: actions/upload-artifact@v4 + with: + name: unit-test-reports + path: "**/build/reports/tests/" + retention-days: 7 + + ktlint: + name: KtLint + runs-on: ubuntu-latest + # Only on a pull request, because it judges a change rather than a state: + # what reaches `publish` has already been judged on the way in. + if: github.event_name == 'pull_request' + timeout-minutes: 15 + + steps: + - name: Checkout code + uses: actions/checkout@v4 + with: + # The whole history, so the merge base with the target branch exists + # locally and the diff below can be taken against it. + fetch-depth: 0 + + - name: Set up JDK 17 + uses: actions/setup-java@v4 + with: + java-version: "17" + distribution: "temurin" + + # Pinned to the version `local_ci.sh` installs, so the answer here is the + # answer a developer gets on their own machine. A floating version would + # let a release of the linter turn every open pull request red. + - name: Download KtLint + env: + KTLINT_VERSION: "0.50.0" + run: | + curl -sSLO "https://github.com/pinterest/ktlint/releases/download/${KTLINT_VERSION}/ktlint" + chmod +x ktlint + ./ktlint --version + + # Only the files the pull request touched. + # + # This is a fork that tracks upstream, and the inherited tree carries + # around seventy findings — nearly all of them in upstream files (the + # sample applications, the no-op modules, some build scripts). Reformatting + # those would mean a conflict in every one of them at the next upstream + # merge, paid forever, to fix code we do not write. Judging the diff keeps + # what we do write clean without taking on that bill. + # The list reaches ktlint through its own `--patterns-from-stdin`, which + # is what that flag is for: no shell array, nothing that word-splits, and + # no argument-length ceiling for a large pull request. + - name: Lint the files this pull request changed + run: | + base="${{ github.event.pull_request.base.sha }}" + git diff --name-only --diff-filter=ACMR "$base...HEAD" -- '*.kt' '*.kts' \ + | grep -v '/build/' > changed-kotlin-files.txt || true + + if [ ! -s changed-kotlin-files.txt ]; then + echo "This pull request changes no Kotlin files." + exit 0 + fi + + echo "Linting:" + sed 's/^/ /' changed-kotlin-files.txt + ./ktlint --relative --patterns-from-stdin < changed-kotlin-files.txt From f503dd8a2b0bb8768eb6f14384c2e0bfa22b3ed0 Mon Sep 17 00:00:00 2001 From: Fiona Date: Wed, 2 Sep 2026 19:46:48 -0700 Subject: [PATCH 2/2] test(logs): remove a test that was never in the module it sits in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `dd-sdk-android-logs-noop` carries a copy of the logs module's `LogcatLogHandlerJavaTest`, byte for byte. The class it exercises, `LogcatLogHandler`, only exists in `dd-sdk-android-logs`, and the no-op module declares no test dependencies at all, so the file has never compiled there — it came along when the module's directory structure was copied and nothing has run those tests since to notice. Nothing is lost by removing it: the identical file remains in `dd-sdk-android-logs`, where the class and the dependencies are, and it runs there today. Found by the unit test workflow added alongside this, which is what a gate is for. --- .../logger/LogcatLogHandlerJavaTest.java | 77 ------------------- 1 file changed, 77 deletions(-) delete mode 100644 features/dd-sdk-android-logs-noop/src/testDebug/java/com/datadog/android/log/internal/logger/LogcatLogHandlerJavaTest.java diff --git a/features/dd-sdk-android-logs-noop/src/testDebug/java/com/datadog/android/log/internal/logger/LogcatLogHandlerJavaTest.java b/features/dd-sdk-android-logs-noop/src/testDebug/java/com/datadog/android/log/internal/logger/LogcatLogHandlerJavaTest.java deleted file mode 100644 index 486117388e..0000000000 --- a/features/dd-sdk-android-logs-noop/src/testDebug/java/com/datadog/android/log/internal/logger/LogcatLogHandlerJavaTest.java +++ /dev/null @@ -1,77 +0,0 @@ -/* - * Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0. - * This product includes software developed at Datadog (https://www.datadoghq.com/). - * Copyright 2016-Present Datadog, Inc. - */ - -package com.datadog.android.log.internal.logger; - -import fr.xgouchet.elmyr.annotation.StringForgery; -import fr.xgouchet.elmyr.junit5.ForgeExtension; - -import static org.assertj.core.api.Assertions.assertThat; - -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; - -import java.util.concurrent.atomic.AtomicReference; - -@SuppressWarnings("KotlinInternalInJava") -@ExtendWith(ForgeExtension.class) -public class LogcatLogHandlerJavaTest { - - LogcatLogHandler testedHandler; - - @StringForgery - String fakeServiceName; - - @Test - void resolves_stack_trace_element_null_if_in_release_mode() { - testedHandler = new LogcatLogHandler(fakeServiceName, true, false); - - StackTraceElement element = testedHandler.getCallerStackElement$dd_sdk_android_logs_debug(); - - assertThat(element) - .isNull(); - } - - @Test - void resolves_stack_trace_element_null_if_useClassnameAsTag_is_false() { - testedHandler = new LogcatLogHandler(fakeServiceName, false, true); - - StackTraceElement element = testedHandler.getCallerStackElement$dd_sdk_android_logs_debug(); - - assertThat(element) - .isNull(); - } - - @Test - void resolves_stack_trace_element_from_caller() { - testedHandler = new LogcatLogHandler(fakeServiceName, true, true); - - StackTraceElement element = testedHandler.getCallerStackElement$dd_sdk_android_logs_debug(); - - assertThat(element).isNotNull(); - assertThat(element.getClassName()) - .isEqualTo(getClass().getCanonicalName()); - } - - @Test - void resolves_nested_stack_trace_element_from_caller() { - testedHandler = new LogcatLogHandler(fakeServiceName, true, true); - - AtomicReference elementRef = new AtomicReference<>(); - - Runnable runnable = new Runnable() { - @Override - public void run() { - elementRef.set(testedHandler.getCallerStackElement$dd_sdk_android_logs_debug()); - } - }; - runnable.run(); - - assertThat(elementRef).isNotNull(); - assertThat(elementRef.get().getClassName()) - .isEqualTo(getClass().getCanonicalName() + "$1"); - } -}