Skip to content
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

[Test Harness] Add test harness library #1392

Merged
merged 24 commits into from Nov 23, 2022
Merged

Conversation

alexvanyo
Copy link
Collaborator

Adds a new testing library, testharness for testing Compose layouts.

@JoseAlcerreca JoseAlcerreca marked this pull request as ready for review November 18, 2022 14:16
@JoseAlcerreca JoseAlcerreca changed the title [WIP] [Test Harness] Add test harness library [Test Harness] Add test harness library Nov 18, 2022
@yschimke
Copy link
Contributor

Want to add isRound? :)

Copy link
Collaborator

@bentrengrove bentrengrove left a comment

Choose a reason for hiding this comment

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

Please add an issue template to .github/ISSUE_TEMPLATE and also an entry for the docs generation into mkdocs.yml

Also, is there no sample for this project?

@alexvanyo
Copy link
Collaborator Author

Want to add isRound? :)

For isRound, I'd want to do more investigation with how the APIs handle rendering outside of the round viewport on a circular watch.

For example: if you render something in the very corner of a physically round watch (such that it isn't actually visible), do the testing APIs treat it as being visible?

If they do, the isRound should be just a matter of flipping the Configuration flag, otherwise we could inset the content within the round viewport.

@alexvanyo
Copy link
Collaborator Author

Please add an issue template to .github/ISSUE_TEMPLATE and also an entry for the docs generation into mkdocs.yml

Added, along with a first draft of documentation as well.

Also, is there no sample for this project?

There are the tests of the TestHarness itself, and I converted some tests in pager and adaptive where it makes sense to use TestHarness, but agreed there should be more.

Do you think it makes sense to add some tests to sample that make use of it? I was thinking a navigation rail versus navigation bar test setup would be a good showcase of it.

Copy link
Collaborator

@bentrengrove bentrengrove left a comment

Choose a reason for hiding this comment

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

LGTM! Amazing work.

For the sample app, if you don't want to add a test in the sample app, just a note in your docs about where to see a sample in the repo would be good.

@yschimke
Copy link
Contributor

Want to add isRound? :)

For isRound, I'd want to do more investigation with how the APIs handle rendering outside of the round viewport on a circular watch.

For example: if you render something in the very corner of a physically round watch (such that it isn't actually visible), do the testing APIs treat it as being visible?

If they do, the isRound should be just a matter of flipping the Configuration flag, otherwise we could inset the content within the round viewport.

Yep, I can follow up.

The answer is yes, they can draw outside the bounds and screenshot tools show content outside. So for Paparazzi I change both the configuration as you suggest, but also apply a circular crop.

@JoseAlcerreca
Copy link
Collaborator

Given the sample is a catalog, we could add a testharness sample (with a big warning so that it's not used in prod)

image

Once I fix the darkMode issue...

Thoughts?

@JoseAlcerreca
Copy link
Collaborator

I've pushed it anyway, but happy to revert.

@yschimke
Copy link
Contributor

I tried this to see if it could replace my bespoke code. Not sure if it's AGP or paparazzi related (I doubt given it's debug code, not test). But seeing

java.lang.NullPointerException: Cannot invoke "android.content.Context.getResources()" because "resContext" is null   at android.view.ContextThemeWrapper.getResourcesInternal(ContextThemeWrapper.java:137)   at android.view.ContextThemeWrapper.getResources(ContextThemeWrapper.java:128)   at androidx.compose.ui.res.Resources_androidKt.resources(Resources.android.kt:33)   at androidx.compose.ui.res.StringResources_androidKt.stringResource(StringResources.android.kt:35)

Hopefully some typo in my copy/paste/simplification. But wanted to flag.

@JoseAlcerreca
Copy link
Collaborator

Hopefully some typo in my copy/paste/simplification. But wanted to flag.

I can't think of anything that the harness might be doing that might be affecting resource loading. Can you share your code?

@yschimke
Copy link
Contributor

Hopefully some typo in my copy/paste/simplification. But wanted to flag.

I can't think of anything that the harness might be doing that might be affecting resource loading. Can you share your code?

yschimke/horologist@155971e

Don't block on this, mainly flagging in case it's a more general problem.

@yschimke
Copy link
Contributor

For completeness I would suggest one of the tests being to change the locale and call stringResource() from a composable.

@JoseAlcerreca
Copy link
Collaborator

Done
image

@alexvanyo
Copy link
Collaborator Author

I was trying out TestHarness in a @Preview, and ran into a similar looking error.

I believe the issue is the same, caused by this line in layoutlib.

ContextThemeWrapper uses Context.createConfigurationContext to apply the override, and it looks like this isn't supported in layoutlib directly, and instead, null is returned which leads to the crash.

A proper fix would likely be adding some functionality to layoutlib, to support ContextThemeWrapper? https://issuetracker.google.com/issues/200845657 seems similar.

@alexvanyo
Copy link
Collaborator Author

List of updates since your approval @bentrengrove :

  • Added test harness sample
  • Added locale overriding to sample and tests
  • Documented current @Preview limitation

@JoseAlcerreca
Copy link
Collaborator

Got the Trengrove 🚀 of approval offline, merging!

@JoseAlcerreca JoseAlcerreca merged commit 8ce3ce4 into main Nov 23, 2022
@JoseAlcerreca JoseAlcerreca deleted the jaav/dev-testharness branch November 23, 2022 09:56
Comment on lines +104 to +109
// Update the locale list
if (Build.VERSION.SDK_INT >= 24) {
setLocales(LocaleList.forLanguageTags(locales.toLanguageTags()))
} else {
setLocale(locales[0])
}
Copy link

Choose a reason for hiding this comment

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

@alexvanyo java.util.Locale.setDefault(locales[0]) would be also useful here since java formatters won't know about the locale update otherwise

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd be interested to hear your use case here, since I'd intentionally prefer to avoid calling java.util.Locale.setDefault(locales[0]) if possible.

java.util.Locale.getDefault() isn't observable by Compose, so if you're calling that from a @Composable function, you might not be notified if the locale changes in the general case (for instance, if opting out of activity recreation for locale changes)

If possible, can you retrieve the locale from LocalConfiguration.current.locales instead to create the formatter?

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.

None yet

5 participants