You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ambari.apache.org by Vitalyi Brodetskyi <vb...@hortonworks.com> on 2014/02/19 16:25:55 UTC
Review Request 18271: Add unit test that verifies escaped urls(ProxyService)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18271/
-----------------------------------------------------------
Review request for Ambari, Andrew Onischuk and Nate Cole.
Bugs: AMBARI-4738
https://issues.apache.org/jira/browse/AMBARI-4738
Repository: ambari
Description
-------
For completeness, we should have a unit test that verifies escaped urls are covered. IE:
/proxy?url=http%3a%2f%2fserver%3a8188%2fws%2fv1%2fapptimeline%2fHIVE_QUERY_ID%3ffields=events%2cprimaryfilters%26limit=10%26primaryFilter=user%3ahiveuser1
In fact, requiring that syntax gets out of the substring/replace business and just use the url parameter and make a URI out of it.
Diffs
-----
ambari-server/src/test/java/org/apache/ambari/server/proxy/ProxyServiceTest.java b90af09
Diff: https://reviews.apache.org/r/18271/diff/
Testing
-------
-------------------------------------------------------
T E S T S
-------------------------------------------------------
Listening for transport dt_socket at address: 5007
Running org.apache.ambari.server.proxy.ProxyServiceTest
Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 68.465 sec
Results :
Tests run: 8, Failures: 0, Errors: 0, Skipped: 0
Thanks,
Vitalyi Brodetskyi
Re: Review Request 18271: Add unit test that verifies escaped
urls(ProxyService)
Posted by Nate Cole <nc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18271/#review34868
-----------------------------------------------------------
Ship it!
Ship It!
- Nate Cole
On Feb. 19, 2014, 10:25 a.m., Vitalyi Brodetskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18271/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2014, 10:25 a.m.)
>
>
> Review request for Ambari, Andrew Onischuk and Nate Cole.
>
>
> Bugs: AMBARI-4738
> https://issues.apache.org/jira/browse/AMBARI-4738
>
>
> Repository: ambari
>
>
> Description
> -------
>
> For completeness, we should have a unit test that verifies escaped urls are covered. IE:
>
> /proxy?url=http%3a%2f%2fserver%3a8188%2fws%2fv1%2fapptimeline%2fHIVE_QUERY_ID%3ffields=events%2cprimaryfilters%26limit=10%26primaryFilter=user%3ahiveuser1
>
> In fact, requiring that syntax gets out of the substring/replace business and just use the url parameter and make a URI out of it.
>
>
> Diffs
> -----
>
> ambari-server/src/test/java/org/apache/ambari/server/proxy/ProxyServiceTest.java b90af09
>
> Diff: https://reviews.apache.org/r/18271/diff/
>
>
> Testing
> -------
>
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Listening for transport dt_socket at address: 5007
> Running org.apache.ambari.server.proxy.ProxyServiceTest
> Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 68.465 sec
>
> Results :
>
> Tests run: 8, Failures: 0, Errors: 0, Skipped: 0
>
>
> Thanks,
>
> Vitalyi Brodetskyi
>
>
Re: Review Request 18271: Add unit test that verifies escaped
urls(ProxyService)
Posted by Andrew Onischuk <ao...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18271/#review34870
-----------------------------------------------------------
Ship it!
Ship It!
- Andrew Onischuk
On Feb. 19, 2014, 3:25 p.m., Vitalyi Brodetskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18271/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2014, 3:25 p.m.)
>
>
> Review request for Ambari, Andrew Onischuk and Nate Cole.
>
>
> Bugs: AMBARI-4738
> https://issues.apache.org/jira/browse/AMBARI-4738
>
>
> Repository: ambari
>
>
> Description
> -------
>
> For completeness, we should have a unit test that verifies escaped urls are covered. IE:
>
> /proxy?url=http%3a%2f%2fserver%3a8188%2fws%2fv1%2fapptimeline%2fHIVE_QUERY_ID%3ffields=events%2cprimaryfilters%26limit=10%26primaryFilter=user%3ahiveuser1
>
> In fact, requiring that syntax gets out of the substring/replace business and just use the url parameter and make a URI out of it.
>
>
> Diffs
> -----
>
> ambari-server/src/test/java/org/apache/ambari/server/proxy/ProxyServiceTest.java b90af09
>
> Diff: https://reviews.apache.org/r/18271/diff/
>
>
> Testing
> -------
>
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Listening for transport dt_socket at address: 5007
> Running org.apache.ambari.server.proxy.ProxyServiceTest
> Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 68.465 sec
>
> Results :
>
> Tests run: 8, Failures: 0, Errors: 0, Skipped: 0
>
>
> Thanks,
>
> Vitalyi Brodetskyi
>
>
Re: Review Request 18271: Add unit test that verifies escaped
urls(ProxyService)
Posted by Vitalyi Brodetskyi <vb...@hortonworks.com>.
> On Feb. 19, 2014, 3:42 p.m., Nate Cole wrote:
> > The new test method does not make any assertions. You should try using a Capture to verify the string URI's are the same. Also (and this is my fault for not realizing it earlier), a year ago or so, the team agreed to use EasyMock instead of PowerMock. If you can, please move this test class over to use EasyMock.
About assertions. Using expect() method, we can test it too. For example:
URI uri = UriBuilder.fromUri("http://dev01.hortonworks.com:8080/proxy?url=http%3a%2f%2fserver%3a8188%2fws%2fv1%2f" +
+ "apptimeline%2fHIVE_QUERY_ID%3ffields=events%2cprimaryfilters%26limit=10%26primaryFilter=user%3ahiveuser1").build();
expect(getUriInfo().getRequestUri()).andReturn(uri);
expect(streamProviderMock.processURL("http://server:8188/ws/v1/apptimeline/HIVE_QUERY_ID?fields=events,primary" +
+ "filters&limit=10&primaryFilter=user:hiveuser1", "GET", null, headerParamsToForward)).andReturn(urlConnectionMock);
1) Create uri object with undecoded url.
2) Add expect() to return created earlier uri object.
3) Add expect() to return urlConnectionMock. If expect params will not be the same as in test, then we will got NPE, because not urlConnectionMock will be returned, bu null.
About PowerMock. I agree with but that it's more correct to use EasyMock. But i can't mock final classes with EasyMock(URI,ResponseBuilderImpl) and can't mock instance creation like for URLStreamProvider(May be we can add some method to set it).
- Vitalyi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18271/#review34864
-----------------------------------------------------------
On Feb. 19, 2014, 3:25 p.m., Vitalyi Brodetskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18271/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2014, 3:25 p.m.)
>
>
> Review request for Ambari, Andrew Onischuk and Nate Cole.
>
>
> Bugs: AMBARI-4738
> https://issues.apache.org/jira/browse/AMBARI-4738
>
>
> Repository: ambari
>
>
> Description
> -------
>
> For completeness, we should have a unit test that verifies escaped urls are covered. IE:
>
> /proxy?url=http%3a%2f%2fserver%3a8188%2fws%2fv1%2fapptimeline%2fHIVE_QUERY_ID%3ffields=events%2cprimaryfilters%26limit=10%26primaryFilter=user%3ahiveuser1
>
> In fact, requiring that syntax gets out of the substring/replace business and just use the url parameter and make a URI out of it.
>
>
> Diffs
> -----
>
> ambari-server/src/test/java/org/apache/ambari/server/proxy/ProxyServiceTest.java b90af09
>
> Diff: https://reviews.apache.org/r/18271/diff/
>
>
> Testing
> -------
>
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Listening for transport dt_socket at address: 5007
> Running org.apache.ambari.server.proxy.ProxyServiceTest
> Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 68.465 sec
>
> Results :
>
> Tests run: 8, Failures: 0, Errors: 0, Skipped: 0
>
>
> Thanks,
>
> Vitalyi Brodetskyi
>
>
Re: Review Request 18271: Add unit test that verifies escaped
urls(ProxyService)
Posted by Nate Cole <nc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18271/#review34864
-----------------------------------------------------------
The new test method does not make any assertions. You should try using a Capture to verify the string URI's are the same. Also (and this is my fault for not realizing it earlier), a year ago or so, the team agreed to use EasyMock instead of PowerMock. If you can, please move this test class over to use EasyMock.
- Nate Cole
On Feb. 19, 2014, 10:25 a.m., Vitalyi Brodetskyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18271/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2014, 10:25 a.m.)
>
>
> Review request for Ambari, Andrew Onischuk and Nate Cole.
>
>
> Bugs: AMBARI-4738
> https://issues.apache.org/jira/browse/AMBARI-4738
>
>
> Repository: ambari
>
>
> Description
> -------
>
> For completeness, we should have a unit test that verifies escaped urls are covered. IE:
>
> /proxy?url=http%3a%2f%2fserver%3a8188%2fws%2fv1%2fapptimeline%2fHIVE_QUERY_ID%3ffields=events%2cprimaryfilters%26limit=10%26primaryFilter=user%3ahiveuser1
>
> In fact, requiring that syntax gets out of the substring/replace business and just use the url parameter and make a URI out of it.
>
>
> Diffs
> -----
>
> ambari-server/src/test/java/org/apache/ambari/server/proxy/ProxyServiceTest.java b90af09
>
> Diff: https://reviews.apache.org/r/18271/diff/
>
>
> Testing
> -------
>
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Listening for transport dt_socket at address: 5007
> Running org.apache.ambari.server.proxy.ProxyServiceTest
> Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 68.465 sec
>
> Results :
>
> Tests run: 8, Failures: 0, Errors: 0, Skipped: 0
>
>
> Thanks,
>
> Vitalyi Brodetskyi
>
>