-
Notifications
You must be signed in to change notification settings - Fork 3k
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
base: webview
Are you sure you want to change the base?
Testing #158
Conversation
…nflicts are avoided
WebView/app/src/androidTest/java/com/android/samples/webviewdemo/ExampleInstrumentedTest.kt
Outdated
Show resolved
Hide resolved
… if dark more is supported
Change data source
… if dark more is supported
There was a problem hiding this 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, | ||
jsObjName, | ||
allowedOriginRules | ||
) { message -> MainActivity.invokeShareIntent(message) } |
There was a problem hiding this comment.
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>" |
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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?
WebView/app/src/androidTest/java/com/android/samples/webviewdemo/MainActivityTest.kt
Outdated
Show resolved
Hide resolved
|
||
|
||
// check that method is running on the UI thread | ||
assertTrue(isUiThread()) |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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:
- Event Handling and Threading
- WebView team's doc about threading (this gets into the weeds of WebView implementation, so feel free to skip parts which don't make sense).
|
||
|
||
// Inject JsObject into Html by loading it in the webview | ||
webView.loadData(htmlPage, "text/html", "UTF-8") |
There was a problem hiding this comment.
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.
There was a problem hiding this 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 😄
val webView = WebView(context) | ||
// Create JsObject | ||
createJsObject( | ||
webView, | ||
jsObjName, | ||
allowedOriginRules | ||
) { message -> //save message; call .set() | ||
} | ||
run() { |
There was a problem hiding this comment.
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, () -> {
// ...
});
// ...
});
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- "UTF-8" is not the right thing for
encoding
. Passnull
instead. See https://developer.android.com/reference/kotlin/android/webkit/WebView#loaddata for what this parameter actually means - It's OK to use the same baseURL for the
historyUrl
, but it would probably be fine to just passnull
. This parameter isn't very meaningful for our case - Please add a space after each comma
There was a problem hiding this comment.
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()) } |
There was a problem hiding this comment.
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() } |
There was a problem hiding this comment.
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).
Custom dark theme
Fix Share Feature
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.