Skip to content

ci: run the library unit tests and the formatter on every pull request - #31

Open
Fiona2016 wants to merge 2 commits into
publishfrom
ci/unit-tests-and-ktlint
Open

ci: run the library unit tests and the formatter on every pull request#31
Fiona2016 wants to merge 2 commits into
publishfrom
ci/unit-tests-and-ktlint

Conversation

@Fiona2016

Copy link
Copy Markdown
Collaborator

Why

The only workflow reaching a pull request was CodeQL. Nothing ran the unit 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

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, as that workflow's own comments record, and their tests are not what a library change needs to be judged on. Forty module test tasks, so the timeout is generous.

KtLint, on the diff rather than the tree

This is a fork that tracks upstream, and the inherited tree carries 68 findings — nearly all of them in files we do not write:

file findings
sample/kotlin/.../SampleApplication.kt 12
.../session-replay-noop/MobileSegmentConstants.kt 10
.../flags-noop/FlagsClient.kt 10
build scripts, other no-op modules, tools the rest

Reformatting those would buy a conflict in every one of them at the next upstream merge, paid forever, to fix code that is not ours. Judging the files a pull request changed keeps what we do write clean without taking on that bill.

The ktlint version is pinned to the one 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.

Detekt is deliberately absent

Two separate things wear that name here, and neither fits in this change:

  • Shared rules (detekt-common.yml, detekt-public-api.yml) live in a repository this project does not have access to — local_ci.sh exits if DD_SOURCE is unset.
  • Custom rules (detekt_custom_*.yml, detekt_test_pyramid.yml) do live here, but need assembleLibrariesDebug plus a built plugin jar first, for type resolution. That is a far heavier job than either gate above and deserves its own decision rather than being smuggled in behind them.

Verification

Everything here was exercised locally rather than assumed:

  • actionlint clean — and confirmed it was actually inspecting the run: blocks by feeding it a script with a known shellcheck finding, which it rejected. A linter that passes because it is not looking is worth nothing.
  • The ktlint step's exact command was run against a real branch: it selected the 23 changed Kotlin files and reported findings, and a clean file exits 0 while a file with findings exits 1.
  • An earlier draft used mapfile, which does not exist in bash 3.2 and so could not be exercised outside the runner; it was replaced with ktlint's own --patterns-from-stdin, which also removes the argument-length ceiling for a large pull request.
  • ./gradlew unitTestDebug --dry-run resolves to 40 module test tasks.

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.
`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.
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