Skip to content

200: Remove the unused useLocale project property - #329

Open
DanielJette wants to merge 1 commit into
mainfrom
200-remove-use-locale
Open

DanielJette wants to merge 1 commit into
mainfrom
200-remove-use-locale

Conversation

@DanielJette

Copy link
Copy Markdown
Contributor

What does this change accomplish?

Fixes #200

-PuseLocale=en_CA has no effect, as reported. The cause is that the property is declared and never
read by anything:

$ grep -rn useLocale --include=*.kt .
Plugins/Gradle/src/main/kotlin/dev/testify/internal/GradleProjectExtensions.kt:42
Plugins/Gradle/src/main/kotlin/dev/testify/internal/GradleProjectExtensions.kt:43

Those two lines are the declaration itself. There are no other references in the plugin, the library
or any extension — it is never forwarded as an AdbParam and never reaches the device.

It also could not have worked as the issue expects even if it were wired up. The property is typed
Boolean, so -PuseLocale=en_CA goes through "en_CA".toBoolean() and evaluates to false.

How have you achieved it?

Removed the property rather than implementing it.

Implementing a suite-wide locale override is not a small change, and it is not obviously the right
feature. Locale is part of the baseline device key — DEFAULT_FOLDER_FORMAT is "a-wxh@d-l" — so a
suite-wide override relocates the entire baseline directory, and it would have to lose to a per-test
TestifyConfiguration.locale wherever both are set. That is a behaviour change worth designing
deliberately, not a bug fix.

The supported path already exists and is per-test, which is what screenshot baselines want anyway:
TestifyConfiguration.locale, documented in
Changing the Locale in a test. The Samples/Flix RTL test
added in #327 exercises it, producing a separate …-fa baseline directory alongside the …-en_US
one — the per-test mechanism working as intended.

Also removed the settings.md row that documented the property as "accepted, but the library doesn't
currently use it", since there is no longer anything to accept.

Scope of Impact and Testing instructions

A build passing -PuseLocale will no longer have the property resolve inside the plugin. Nothing
observable changes, because nothing read it. Not breaking in practice: the flag was already a no-op,
and Gradle ignores unrecognised -P properties rather than failing.

CHANGELOG entry added under Unreleased.

Verified locally:

./gradlew Plugin:ktlintCheck Plugin:test Plugin:assemble   # BUILD SUCCESSFUL
cd docs && npm run build                                   # clean, onBrokenLinks = throw

@AndroidTestifyBot AndroidTestifyBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved — but CI needs a re-run before merge

Verified

  • useLocale has no reader anywhere: the only hits in the repo are the declaration itself and the settings.md row, both removed here. Nothing forwards it as an AdbParam.
  • Plugin:ktlintCheck Plugin:test Plugin:assemble pass locally, and the docs build is clean.
  • Removing rather than implementing is the right call for the reasons given; locale is part of the device key, so a suite-wide override is a design decision, not a bug fix.

CI is red on this PR and that needs resolving, not ignoring
Legacy Sample failed on Bitrise and Library / Plugin are stuck at pending. I could not reproduce it: ./gradlew LegacySample:screenshotTest on this branch against an API 37 emulator gives OK (88 tests). Nothing in this diff can reach the Legacy sample at runtime, and #330 shows the identical pattern from the same ten-minute window, so this looks like infrastructure. Please re-run the pipeline and merge only on green.

Non-blocking

  • Project.useLocale was a public extension property. It lives in dev.testify.internal, so I don't think it needs a "Breaking" label, but the CHANGELOG entry could say "public but internal-package" for anyone who reads it.
  • The CHANGELOG hunk lands on the same line as #330, #331, #332, #333 and #335; each merge will conflict the next.

This branch has not been deployed

No deployments
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.

Gradle project property useLocale is not used

2 participants