Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2670643002:
[Android] Refactor LoadUrlTest to replace package org.apache.http (Closed)
|
Created:
3 years, 10 months ago by shenghuazhang Modified:
3 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, android-webview-reviews_chromium.org, net-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Refactor LoadUrlTest to replace package org.apache.http
Refactor 5 tests to get rid of org.apache.http related packages.
To make it happens, add a default handler in embedded_test_server native code for test
testRendererNavigationAndGoBackWithExtraHeaders. Which should handle
renderer navigation via clicking redirection, as well as echo the header of the
requested url. (bug=686247
)
BUG=488194
This CL is covered by https://codereview.chromium.org/2687573002/. Close it.
Patch Set 1 #
Total comments: 10
Patch Set 2 : split ets custom handler related tests out #
Total comments: 10
Patch Set 3 : nit & standardSetup #
Total comments: 2
Patch Set 4 : remove standardSetup #
Total comments: 10
Messages
Total messages: 16 (3 generated)
shenghuazhang@chromium.org changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/2670643002/diff/1/android_webview/javatests/s... File android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java (right): https://codereview.chromium.org/2670643002/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:162: validateHeaderNone(this, awContents, contentsClient); Doubt about ``` loadUrlSync(awContents, contentsClient.getOnPageFinishedHelper(), url2); ``` can really verify that extra headers are only passed for the main resource (url1), since if run ``` loadUrlSync(awContents, contentsClient.getOnPageFinishedHelper(), url1); ``` here, it's also proved to be no extra headers any more. If the scenario should be remained, how to deal with the 'checking extra headers for not-main resource' by embedded_test_server?
https://codereview.chromium.org/2670643002/diff/1/android_webview/javatests/s... File android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java (right): https://codereview.chromium.org/2670643002/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:116: private String getJSBodyTextContent(InstrumentationTestCase testCase, Should we have something like this in AwTestBase? You used something similar in a previous version, and I imagine that the /echo* URL conversions will all require something like this. https://codereview.chromium.org/2670643002/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:130: private void validateHeaderNone(InstrumentationTestCase testCase, final AwContents awContents, This is just validateHeaderValue(testCase, awContents, contentsClient, "None"), right? Does it need to be a separate function? https://codereview.chromium.org/2670643002/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:144: EmbeddedTestServer testServer = Given how many tests are using the ETS, I think it'd be ok to create it in setUp and tear it down in tearDown rather than having try/finally in all of these tests. https://codereview.chromium.org/2670643002/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:162: validateHeaderNone(this, awContents, contentsClient); On 2017/02/02 00:03:24, shenghuazhang wrote: > Doubt about > ``` > loadUrlSync(awContents, > contentsClient.getOnPageFinishedHelper(), > url2); > ``` > can really verify that extra headers are only passed for the main resource > (url1), since if run > ``` > loadUrlSync(awContents, > contentsClient.getOnPageFinishedHelper(), > url1); > ``` > here, it's also proved to be no extra headers any more. > > If the scenario should be remained, how to deal with the 'checking extra headers > for not-main resource' by embedded_test_server? Yeah, this can't really verify the non-main resource. I think you'll need a custom handler. https://codereview.chromium.org/2670643002/diff/1/net/test/embedded_test_serv... File net/test/embedded_test_server/default_handlers.cc (right): https://codereview.chromium.org/2670643002/diff/1/net/test/embedded_test_serv... net/test/embedded_test_server/default_handlers.cc:551: std::unique_ptr<HttpResponse> HandleClickRedirect(const HttpRequest& request) { I'm not sure that this should be in the default handler set. I think you should pull this & the conversion of the test that requires it out of this CL for now s.t. we can land the other conversions & start working on exposing custom handler support to java.
https://codereview.chromium.org/2670643002/diff/1/android_webview/javatests/s... File android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java (right): https://codereview.chromium.org/2670643002/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:116: private String getJSBodyTextContent(InstrumentationTestCase testCase, On 2017/02/02 00:46:41, jbudorick wrote: > Should we have something like this in AwTestBase? You used something similar in > a previous version, and I imagine that the /echo* URL conversions will all > require something like this. Done. https://codereview.chromium.org/2670643002/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:130: private void validateHeaderNone(InstrumentationTestCase testCase, final AwContents awContents, On 2017/02/02 00:46:41, jbudorick wrote: > This is just validateHeaderValue(testCase, awContents, contentsClient, "None"), > right? Does it need to be a separate function? Done. https://codereview.chromium.org/2670643002/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:144: EmbeddedTestServer testServer = On 2017/02/02 00:46:41, jbudorick wrote: > Given how many tests are using the ETS, I think it'd be ok to create it in setUp > and tear it down in tearDown rather than having try/finally in all of these > tests. Done. https://codereview.chromium.org/2670643002/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:162: validateHeaderNone(this, awContents, contentsClient); On 2017/02/02 00:46:41, jbudorick wrote: > On 2017/02/02 00:03:24, shenghuazhang wrote: > > Doubt about > > ``` > > loadUrlSync(awContents, > > contentsClient.getOnPageFinishedHelper(), > > url2); > > ``` > > can really verify that extra headers are only passed for the main resource > > (url1), since if run > > ``` > > loadUrlSync(awContents, > > contentsClient.getOnPageFinishedHelper(), > > url1); > > ``` > > here, it's also proved to be no extra headers any more. > > > > If the scenario should be remained, how to deal with the 'checking extra > headers > > for not-main resource' by embedded_test_server? > > Yeah, this can't really verify the non-main resource. I think you'll need a > custom handler. Will split this test with testRendererNavigationAndGoBackWithExtraHeaders to another CL related to ETS customer handler.
shenghuazhang@chromium.org changed reviewers: + sgurun@chromium.org
Add @sgurun in Reviewers list.
https://codereview.chromium.org/2670643002/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java (right): https://codereview.chromium.org/2670643002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:542: protected String getJavasScriptResultBodyTextContent(final AwContents awContents, nit: getJavasScript... -> getJavaScript... https://codereview.chromium.org/2670643002/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java (left): https://codereview.chromium.org/2670643002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:288: settings.setJavaScriptEnabled(true); Why are we calling enableJavaScriptOnUiThread in the other tests if they weren't previously calling this? https://codereview.chromium.org/2670643002/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java (right): https://codereview.chromium.org/2670643002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:42: mContentsClient = new TestAwContentsClient(); What's the motivation behind pulling these into setUp? I'm a little wary of the case below that resets mAwContents. https://codereview.chromium.org/2670643002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:96: final AwContents mAwContents, This should stay awContents. https://codereview.chromium.org/2670643002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:127: private void validateHeaderValue(final AwContents mAwContents, as should this.
https://codereview.chromium.org/2670643002/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java (right): https://codereview.chromium.org/2670643002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:542: protected String getJavasScriptResultBodyTextContent(final AwContents awContents, On 2017/02/03 03:30:09, jbudorick wrote: > nit: getJavasScript... -> getJavaScript... Done. https://codereview.chromium.org/2670643002/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java (left): https://codereview.chromium.org/2670643002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:288: settings.setJavaScriptEnabled(true); On 2017/02/03 03:30:09, jbudorick wrote: > Why are we calling enableJavaScriptOnUiThread in the other tests if they weren't > previously calling this? Because those test cases now either call validateHeaderValue or getJavasScriptResultBodyTextContent which eventually call JSUtils.executeJavaScriptAndWaitForResult. https://codereview.chromium.org/2670643002/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java (right): https://codereview.chromium.org/2670643002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:42: mContentsClient = new TestAwContentsClient(); On 2017/02/03 03:30:09, jbudorick wrote: > What's the motivation behind pulling these into setUp? > > I'm a little wary of the case below that resets mAwContents. Oh that's right. I would separate those to a standard test setup method, and calls it in each test when needs. https://codereview.chromium.org/2670643002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:96: final AwContents mAwContents, On 2017/02/03 03:30:09, jbudorick wrote: > This should stay awContents. Done. https://codereview.chromium.org/2670643002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:127: private void validateHeaderValue(final AwContents mAwContents, On 2017/02/03 03:30:09, jbudorick wrote: > as should this. Done.
Looks pretty good. Just one comment. https://codereview.chromium.org/2670643002/diff/40001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java (right): https://codereview.chromium.org/2670643002/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:51: private void standardSetup() throws Throwable { While this does address my concern about resetting mAwContents, I'm still not sure that this change belongs in this CL: - it's not really related to the server conversion - I'm not sure that it makes the tests more readable.
https://codereview.chromium.org/2670643002/diff/40001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java (right): https://codereview.chromium.org/2670643002/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:51: private void standardSetup() throws Throwable { On 2017/02/03 20:23:39, jbudorick wrote: > While this does address my concern about resetting mAwContents, I'm still not > sure that this change belongs in this CL: > - it's not really related to the server conversion > - I'm not sure that it makes the tests more readable. I picked up the idea from AwContentsClientShouldOverrideUrlLoading.java, which I thought has cleaner code format? But yeah it's not related to server conversion, will remain the origin format =v=
lgtm
https://codereview.chromium.org/2670643002/diff/60001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java (left): https://codereview.chromium.org/2670643002/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:220: createHeadersList(new String[] { "cache-control", "no-cache, no-store" })); these are dropped. Are these set by default by the embedded test server? https://codereview.chromium.org/2670643002/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:223: "x-extraHeaders2", "EXTRA-HEADER-DATA2" why did you drop the second header? https://codereview.chromium.org/2670643002/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:254: createHeadersList(new String[] { "cache-control", "no-cache, no-store" })); same as above. why do we drop cache control headers? https://codereview.chromium.org/2670643002/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:259: "x-extraHeaders2", "EXTRA-HEADER-DATA2" again, why drop the second header?
@sgurun thanks for pointing those questions out! I replied to your comments and needed some confirmation based on the test scenarios. Please have a look at them. If the custom handlers are required for it, will separate these tests changes to a new CL related to embedded_test_server. https://codereview.chromium.org/2670643002/diff/60001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java (left): https://codereview.chromium.org/2670643002/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:220: createHeadersList(new String[] { "cache-control", "no-cache, no-store" })); On 2017/02/03 23:49:07, sgurun wrote: > these are dropped. Are these set by default by the embedded test server? Current embedded_test_server default handler only gets ‘no-cache’ as default cache-control setting: https://codesearch.chromium.org/chromium/src/net/test/embedded_test_server/de... If required to be with specific ‘no-cache, no-store’, then I would handle it by adding and registering custom handler for it in embedded_test_server native side and bind it with java code. Could separate this test to another CL which is embedded_test_server related. https://codereview.chromium.org/2670643002/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:223: "x-extraHeaders2", "EXTRA-HEADER-DATA2" On 2017/02/03 23:49:07, sgurun wrote: > why did you drop the second header? Similar reason. ETS default handler only supports echoing single header in the respond message body. And I thought one header set could be enough to validate the extra header behavior? If i was wrong and checking multiple headers is the must scenario then I will handle with it.
https://codereview.chromium.org/2670643002/diff/60001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java (left): https://codereview.chromium.org/2670643002/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:220: createHeadersList(new String[] { "cache-control", "no-cache, no-store" })); On 2017/02/04 03:16:45, shenghuazhang wrote: > On 2017/02/03 23:49:07, sgurun wrote: > > these are dropped. Are these set by default by the embedded test server? > > Current embedded_test_server default handler only gets ‘no-cache’ as default > cache-control setting: > https://codesearch.chromium.org/chromium/src/net/test/embedded_test_server/de... > > If required to be with specific ‘no-cache, no-store’, then I would handle it by > adding and registering custom handler for it in embedded_test_server native side > and bind it with java code. Could separate this test to another CL which is > embedded_test_server related. I think no-cache should suffice. https://codereview.chromium.org/2670643002/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:223: "x-extraHeaders2", "EXTRA-HEADER-DATA2" On 2017/02/04 03:16:45, shenghuazhang wrote: > On 2017/02/03 23:49:07, sgurun wrote: > > why did you drop the second header? > > Similar reason. ETS default handler only supports echoing single header in the > respond message body. And I thought one header set could be enough to validate > the extra header behavior? If i was wrong and checking multiple headers is the > must scenario then I will handle with it. I think it is better to validate multiple headers as before. We probably run into a scenario where one header was dropped, and this is why the test writer did it. Let's keep it this way.
Several methods overlap with CL https://codereview.chromium.org/2687573002/, would merge this to that one. https://codereview.chromium.org/2670643002/diff/60001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java (left): https://codereview.chromium.org/2670643002/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:220: createHeadersList(new String[] { "cache-control", "no-cache, no-store" })); On 2017/02/07 02:47:33, sgurun wrote: > On 2017/02/04 03:16:45, shenghuazhang wrote: > > On 2017/02/03 23:49:07, sgurun wrote: > > > these are dropped. Are these set by default by the embedded test server? > > > > Current embedded_test_server default handler only gets ‘no-cache’ as default > > cache-control setting: > > > https://codesearch.chromium.org/chromium/src/net/test/embedded_test_server/de... > > > > If required to be with specific ‘no-cache, no-store’, then I would handle it > by > > adding and registering custom handler for it in embedded_test_server native > side > > and bind it with java code. Could separate this test to another CL which is > > embedded_test_server related. > > I think no-cache should suffice. Acknowledged. https://codereview.chromium.org/2670643002/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:223: "x-extraHeaders2", "EXTRA-HEADER-DATA2" On 2017/02/07 02:47:33, sgurun wrote: > On 2017/02/04 03:16:45, shenghuazhang wrote: > > On 2017/02/03 23:49:07, sgurun wrote: > > > why did you drop the second header? > > > > Similar reason. ETS default handler only supports echoing single header in the > > respond message body. And I thought one header set could be enough to validate > > the extra header behavior? If i was wrong and checking multiple headers is the > > must scenario then I will handle with it. > > I think it is better to validate multiple headers as before. We probably run > into a scenario where one header was dropped, and this is why the test writer > did it. Let's keep it this way. Thanks for the clarification. Will keep the multiple headers checking.
Description was changed from ========== [Android] Refactor LoadUrlTest to replace package org.apache.http Refactor 5 tests to get rid of org.apache.http related packages. To make it happens, add a default handler in embedded_test_server native code for test testRendererNavigationAndGoBackWithExtraHeaders. Which should handle renderer navigation via clicking redirection, as well as echo the header of the requested url. (bug=686247) BUG=488194 ========== to ========== [Android] Refactor LoadUrlTest to replace package org.apache.http Refactor 5 tests to get rid of org.apache.http related packages. To make it happens, add a default handler in embedded_test_server native code for test testRendererNavigationAndGoBackWithExtraHeaders. Which should handle renderer navigation via clicking redirection, as well as echo the header of the requested url. (bug=686247) BUG=488194 This CL is covered by https://codereview.chromium.org/2687573002/. Close it. ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
