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

Testing #158

Draft
wants to merge 58 commits into
base: webview
Choose a base branch
from
Draft

Testing #158

wants to merge 58 commits into from

Conversation

gcoleman799
Copy link
Contributor

This PR adds two tests. One checks that jsObject is injected and contains postMessage(). The other checks that the drop down menu behaves as expected.

Copy link
Contributor

@nfischer nfischer left a comment

Choose a reason for hiding this comment

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

Looks like a good start! I left some comments which will hopefully set you in the right direction. Feel free to ping me if anything doesn't make sense.

WebView/app/build.gradle Outdated Show resolved Hide resolved
webView,
jsObjName,
allowedOriginRules
) { message -> MainActivity.invokeShareIntent(message) }
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll want to change this line. Instead of invoking the share intent, you should save message somewhere. I would use a SettableFuture<> for this (call .set() to save the value there, call .get() afterward to block and retrieve the value). Here's a good example test case.

I'm not sure if there's a coroutine-friendly way to do this.

val allowedOriginRules = setOf<String>("https://example.com")

// Create HTML
val htmlPage = "<!DOCTYPE html><html><body>" + " <script>" + " myObject.postMessage('hello');" + " </script>" + "</body></html>"
Copy link
Contributor

Choose a reason for hiding this comment

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

Swap myObject with jsObject. Or even better, you could use string interpolation:

val htmlPage = """
<!DOCTYPE html>
<html>
<body>
<script>${jsObjName}.postMessage('hello');</script>
</body>
</html>
"""

See https://kotlinlang.org/docs/reference/idioms.html#string-interpolation and https://realkotlin.com/tutorials/2018-06-26-multiline-string-literals-in-kotlin/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that it is easier to read if we do something along these lines and get rid fo the htmlPage

webView.loadData("<html></html>","text/html", "UTF-8") webView.evaluateJavascript("myObject.postMessage(someMessage)", null)
But just to check my understanding, with consideration to this comment though the second line becomes
webView.evaluateJavascript("${jsObjName}.postMessage(someMessage)", null)

Is that correct?



// check that method is running on the UI thread
assertTrue(isUiThread())
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want this assert here because instrumentation tests run on the "test thread," not the UI thread. I wrote an internal google doc which goes into more details about this.

As a first step, try moving this into createJsObject's callback. That will return correct results, but comes with the drawback that failing assertion crashes the process instead of showing up as a failed test (the google doc has more context on this). If you can get that working, then you can improve this further by using a SettableFuture<Boolean> to plumb the value across threads (see my previous comment for pointers).

) { message -> MainActivity.invokeShareIntent(message) }

//Inject JsObject into Html
webView.loadData(htmlPage, "text/html", "UTF-8")
Copy link
Contributor

Choose a reason for hiding this comment

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

Android Views are all single-threaded, so they should only be interacted with on the UI thread, not the test thread. In WebView team's own tests, we have our own helper class to post stuff to the UI thread. In your case, you could just copy the second suggestion from https://stackoverflow.com/a/11125271 (the one with new Handler(Looper.getMainLooper()).

Here's some resources:



// Inject JsObject into Html by loading it in the webview
webView.loadData(htmlPage, "text/html", "UTF-8")
Copy link
Contributor

Choose a reason for hiding this comment

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

You should swap this out for loadDataWithBaseUrl and use https://example.com" as the baseUrl. Otherwise, the allowedOriginRules may prevent the object from being injected.

Copy link
Contributor

@nfischer nfischer left a comment

Choose a reason for hiding this comment

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

Looks like it's getting closer 😄

Comment on lines 97 to 105
val webView = WebView(context)
// Create JsObject
createJsObject(
webView,
jsObjName,
allowedOriginRules
) { message -> //save message; call .set()
}
run() {
Copy link
Contributor

Choose a reason for hiding this comment

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

All this should be inside the runnable's "run()" method.


nit: there should be a way to rewrite this to be more like a lambda style:

// This is how it would look in Java
mainHandler.post(() -> {
  WebView webView = WebView(context);
  createJsObject(webView, jsObjName, allowedOriginRules, () -> {
    // ...
  });
  // ...
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will move that and make the change to a lambda style.

// Setup
val jsObjName = "jsObject"
val allowedOriginRules = setOf<String>("https://example.com")
val message = "hello"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: expectedMessage would be better, not to be confused with the "actual message" (the one which comes from out of the callback)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. This made me think that we could also remove the message value in checkingThreadCallbackRunsOn() as it has no purpose there. Because we removed message, I changed the value in the postMessage() call to be the string hello instead of ${message}.

run() {
//Inject JsObject into Html
webView.loadDataWithBaseURL("https://example.com","<html></html>",
"text/html", "UTF-8","https://example.com")
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

webView,
jsObjName,
allowedOriginRules
) { message -> assertTrue(isUiThread()) }
Copy link
Contributor

Choose a reason for hiding this comment

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

The next thing we'll want to do is to use a Future<Boolean> to store this value, and then call assertTrue(future.get()) after we call Handler.post()

// Setup
val jsObjName = "jsObject"
val allowedOriginRules = setOf<String>("https://example.com")
val message = "hello"
val callback = async { isUiThread() }
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this again: how does this control when the coroutine executes? I wonder if the coroutine is executing on its own before the callback happens, which might explain test failures (the code is running on a non-UI thread, so the assertion would fail).

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

4 participants