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

Implement idling resources for tests. #599

Merged
merged 1 commit into from Jun 13, 2023

Conversation

m-sasha
Copy link

@m-sasha m-sasha commented Jun 9, 2023

Proposed Changes

Implement support for idling resources in SkikoComposeUiTest.
Note that the typical way idling resources are used requires sleep (and doesn't make sense in a single-threaded environment), so I added it as an expect/actual, with the JS implementation raising an exception.

We can consider supporting idling resources for JS, but it would require a different API.

Testing

Test: Added a unit test.

@m-sasha m-sasha requested review from igordmn and eymar June 9, 2023 12:13
@@ -137,4 +140,45 @@ class BasicTestTest {
}
}
}

@Test
fun testIdlingResource() {
Copy link
Collaborator

@eymar eymar Jun 9, 2023

Choose a reason for hiding this comment

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

Could this test live in "skikoTest" with @IgnoreJsTarget to run it for k/native too?
In skikoTests, instead of the rule there is a method "runSkikoComposeUiTest" in the scope of which setContent and registerIdlingResource should be available.
If more tests from in this file can be in "skikoTests", then maybe it's better to do it in a separate PR.

Copy link
Author

Choose a reason for hiding this comment

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

Right now I'm working on replicating Android's test API (which uses rule), so I prefer to test with that. Later on, we can work on commonizing the test API, and we'll probably move/copy some tests into common (without rule).

@@ -243,6 +257,9 @@ class SkikoComposeUiTest(
while (!isIdle()) {
renderNextFrame()
uncaughtExceptionHandler.throwUncaught()
if (!areAllResourcesIdle()) {
delay(IDLING_RESOURCES_CHECK_INTERVAL_MS)
Copy link
Collaborator

@eymar eymar Jun 9, 2023

Choose a reason for hiding this comment

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

so JS tests should avoid using "waitForIdle" and use "awaitIdle" instead within TestScope (coroutine scope). Then idling resources should work there too, right?

Copy link
Author

@m-sasha m-sasha Jun 9, 2023

Choose a reason for hiding this comment

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

That's what I was thinking, but the problem is that all the automatic synchronization (following setContent and when you do onNode(...)) calls waitForIdle. So to use it with JS we'll need a new, async, API for those calls.

Copy link
Collaborator

@eymar eymar left a comment

Choose a reason for hiding this comment

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

LGTM! the comments are just questions/suggestions.

@m-sasha m-sasha force-pushed the m-sasha/implement-idling-resources branch from af278e5 to ba3e64f Compare June 11, 2023 12:57
@m-sasha m-sasha merged commit 2a8c2ad into m-sasha/test-framework Jun 13, 2023
2 checks passed
@m-sasha m-sasha deleted the m-sasha/implement-idling-resources branch June 13, 2023 20:45
m-sasha added a commit that referenced this pull request Jun 13, 2023
m-sasha added a commit that referenced this pull request Jun 26, 2023
m-sasha added a commit that referenced this pull request Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants