You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@griffin.apache.org by justACT <gi...@git.apache.org> on 2018/12/02 06:04:12 UTC

[GitHub] incubator-griffin pull request #463: Dev

GitHub user justACT opened a pull request:

    https://github.com/apache/incubator-griffin/pull/463

    Dev

    GRIFFIN-216 [Measure] enhance elasticsearch sink, support elasticsearch security login
     and HttpUtilTest for elasticsearch  with user/pwd and without user/pwd.
    maybe this test is not easy to write, for elasticsearch just use http client and I think the " http client" have been tested by its liberary already.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/justACT/incubator-griffin dev

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-griffin/pull/463.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #463
    
----
commit dd6e2ad5da050ebf3b4a368c73767d154a602790
Author: xiangrchen <xi...@...>
Date:   2018-11-27T07:26:45Z

    enhance elasticsearch sink, support elasticsearch security login

commit 5e1afefcc02adeefe7c50a934138608975584316
Author: xiangrchen <xi...@...>
Date:   2018-11-29T13:33:23Z

    Merge branch 'justACT/master' into justACT/dev
    merge code: enhance elasticsearch sink, support elasticsearch security login

commit 521c3dea157eb232b890beaad9a255bcce07edab
Author: xiangrchen <xi...@...>
Date:   2018-12-01T03:28:44Z

    Merge branch 'master' of https://github.com/apache/incubator-griffin into dev

commit f4b0be1c2bac68d5c38a9557ce47d3dd7222e2f2
Author: xiangrchen <xi...@...>
Date:   2018-12-02T05:56:26Z

    add HttpUtilTest, used by elasticsearch

----


---

[GitHub] incubator-griffin pull request #463: Dev

Posted by justACT <gi...@git.apache.org>.
Github user justACT commented on a diff in the pull request:

    https://github.com/apache/incubator-griffin/pull/463#discussion_r238093362
  
    --- Diff: measure/src/test/scala/org/apache/griffin/measure/utils/HttpUtilTest.scala ---
    @@ -0,0 +1,63 @@
    +/*
    +Licensed to the Apache Software Foundation (ASF) under one
    +or more contributor license agreements.  See the NOTICE file
    +distributed with this work for additional information
    +regarding copyright ownership.  The ASF licenses this file
    +to you under the Apache License, Version 2.0 (the
    +"License"); you may not use this file except in compliance
    +with the License.  You may obtain a copy of the License at
    +
    +  http://www.apache.org/licenses/LICENSE-2.0
    +
    +Unless required by applicable law or agreed to in writing,
    +software distributed under the License is distributed on an
    +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    +KIND, either express or implied.  See the License for the
    +specific language governing permissions and limitations
    +under the License.
    +*/
    +package org.apache.griffin.measure.utils
    +
    +import org.scalatest.{FlatSpec, Matchers}
    +
    +class HttpUtilTest extends FlatSpec with Matchers {
    +  val url = "http://10.148.181.248:39200/griffin/accuracy"
    +  val method = "post"
    +  val user = ""
    +  val password = ""
    +  val params = Map[String, Object]()
    +  val header = Map[String, Object](("Content-Type", "application/json"))
    +  val data = "{}"
    +
    +  "postData httpReq" should "be with auth" in {
    +    try {
    +      HttpUtil.postData(url, user, password, params, header, data)
    +    } catch {
    +      case e: Throwable => println("cannot connect http client." + e)
    +    }
    +  }
    --- End diff --
    
    in scala, how to mock httpReq.postData(data).asString, could you give an example


---

[GitHub] incubator-griffin pull request #463: Dev

Posted by justACT <gi...@git.apache.org>.
Github user justACT commented on a diff in the pull request:

    https://github.com/apache/incubator-griffin/pull/463#discussion_r238099963
  
    --- Diff: measure/src/test/scala/org/apache/griffin/measure/utils/HttpUtilTest.scala ---
    @@ -0,0 +1,63 @@
    +/*
    +Licensed to the Apache Software Foundation (ASF) under one
    +or more contributor license agreements.  See the NOTICE file
    +distributed with this work for additional information
    +regarding copyright ownership.  The ASF licenses this file
    +to you under the Apache License, Version 2.0 (the
    +"License"); you may not use this file except in compliance
    +with the License.  You may obtain a copy of the License at
    +
    +  http://www.apache.org/licenses/LICENSE-2.0
    +
    +Unless required by applicable law or agreed to in writing,
    +software distributed under the License is distributed on an
    +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    +KIND, either express or implied.  See the License for the
    +specific language governing permissions and limitations
    +under the License.
    +*/
    +package org.apache.griffin.measure.utils
    +
    +import org.scalatest.{FlatSpec, Matchers}
    +
    +class HttpUtilTest extends FlatSpec with Matchers {
    +  val url = "http://10.148.181.248:39200/griffin/accuracy"
    +  val method = "post"
    +  val user = ""
    +  val password = ""
    +  val params = Map[String, Object]()
    +  val header = Map[String, Object](("Content-Type", "application/json"))
    +  val data = "{}"
    +
    +  "postData httpReq" should "be with auth" in {
    +    try {
    +      HttpUtil.postData(url, user, password, params, header, data)
    +    } catch {
    +      case e: Throwable => println("cannot connect http client." + e)
    +    }
    +  }
    --- End diff --
    
    ok, got it.


---

[GitHub] incubator-griffin pull request #463: Dev

Posted by bhlx3lyx7 <gi...@git.apache.org>.
Github user bhlx3lyx7 commented on a diff in the pull request:

    https://github.com/apache/incubator-griffin/pull/463#discussion_r238088474
  
    --- Diff: measure/src/test/scala/org/apache/griffin/measure/utils/HttpUtilTest.scala ---
    @@ -0,0 +1,63 @@
    +/*
    +Licensed to the Apache Software Foundation (ASF) under one
    +or more contributor license agreements.  See the NOTICE file
    +distributed with this work for additional information
    +regarding copyright ownership.  The ASF licenses this file
    +to you under the Apache License, Version 2.0 (the
    +"License"); you may not use this file except in compliance
    +with the License.  You may obtain a copy of the License at
    +
    +  http://www.apache.org/licenses/LICENSE-2.0
    +
    +Unless required by applicable law or agreed to in writing,
    +software distributed under the License is distributed on an
    +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    +KIND, either express or implied.  See the License for the
    +specific language governing permissions and limitations
    +under the License.
    +*/
    +package org.apache.griffin.measure.utils
    +
    +import org.scalatest.{FlatSpec, Matchers}
    +
    +class HttpUtilTest extends FlatSpec with Matchers {
    +  val url = "http://10.148.181.248:39200/griffin/accuracy"
    +  val method = "post"
    +  val user = ""
    +  val password = ""
    +  val params = Map[String, Object]()
    +  val header = Map[String, Object](("Content-Type", "application/json"))
    +  val data = "{}"
    +
    +  "postData httpReq" should "be with auth" in {
    +    try {
    +      HttpUtil.postData(url, user, password, params, header, data)
    +    } catch {
    +      case e: Throwable => println("cannot connect http client." + e)
    +    }
    +  }
    --- End diff --
    
    All the test cases are not verified, which is meaningless in unit test.


---

[GitHub] incubator-griffin pull request #463: Dev

Posted by bhlx3lyx7 <gi...@git.apache.org>.
Github user bhlx3lyx7 commented on a diff in the pull request:

    https://github.com/apache/incubator-griffin/pull/463#discussion_r238091939
  
    --- Diff: measure/src/test/scala/org/apache/griffin/measure/utils/HttpUtilTest.scala ---
    @@ -0,0 +1,63 @@
    +/*
    +Licensed to the Apache Software Foundation (ASF) under one
    +or more contributor license agreements.  See the NOTICE file
    +distributed with this work for additional information
    +regarding copyright ownership.  The ASF licenses this file
    +to you under the Apache License, Version 2.0 (the
    +"License"); you may not use this file except in compliance
    +with the License.  You may obtain a copy of the License at
    +
    +  http://www.apache.org/licenses/LICENSE-2.0
    +
    +Unless required by applicable law or agreed to in writing,
    +software distributed under the License is distributed on an
    +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    +KIND, either express or implied.  See the License for the
    +specific language governing permissions and limitations
    +under the License.
    +*/
    +package org.apache.griffin.measure.utils
    +
    +import org.scalatest.{FlatSpec, Matchers}
    +
    +class HttpUtilTest extends FlatSpec with Matchers {
    +  val url = "http://10.148.181.248:39200/griffin/accuracy"
    +  val method = "post"
    +  val user = ""
    +  val password = ""
    +  val params = Map[String, Object]()
    +  val header = Map[String, Object](("Content-Type", "application/json"))
    +  val data = "{}"
    +
    +  "postData httpReq" should "be with auth" in {
    +    try {
    +      HttpUtil.postData(url, user, password, params, header, data)
    +    } catch {
    +      case e: Throwable => println("cannot connect http client." + e)
    +    }
    +  }
    --- End diff --
    
    I don't think there's any static or private function you need to mock, powermock is unnecessary.
    `httpReq.postData(data).asString` is not easy to test here, you only need to mock for this line.
    BTW, the verification is expected for both success response and parameter existence.


---

[GitHub] incubator-griffin pull request #463: Dev

Posted by bhlx3lyx7 <gi...@git.apache.org>.
Github user bhlx3lyx7 commented on a diff in the pull request:

    https://github.com/apache/incubator-griffin/pull/463#discussion_r238088257
  
    --- Diff: measure/src/test/scala/org/apache/griffin/measure/utils/HttpUtilTest.scala ---
    @@ -0,0 +1,63 @@
    +/*
    +Licensed to the Apache Software Foundation (ASF) under one
    +or more contributor license agreements.  See the NOTICE file
    +distributed with this work for additional information
    +regarding copyright ownership.  The ASF licenses this file
    +to you under the Apache License, Version 2.0 (the
    +"License"); you may not use this file except in compliance
    +with the License.  You may obtain a copy of the License at
    +
    +  http://www.apache.org/licenses/LICENSE-2.0
    +
    +Unless required by applicable law or agreed to in writing,
    +software distributed under the License is distributed on an
    +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    +KIND, either express or implied.  See the License for the
    +specific language governing permissions and limitations
    +under the License.
    +*/
    +package org.apache.griffin.measure.utils
    +
    +import org.scalatest.{FlatSpec, Matchers}
    +
    +class HttpUtilTest extends FlatSpec with Matchers {
    +  val url = "http://10.148.181.248:39200/griffin/accuracy"
    --- End diff --
    
    Seems like this ip address is a specific one, which will not work for other environments.
    Actually, all the test cases throw the "connect timed out" exception in my IDE. 
    You need to mock the http request actions for this UT.


---

[GitHub] incubator-griffin pull request #463: Dev

Posted by justACT <gi...@git.apache.org>.
Github user justACT commented on a diff in the pull request:

    https://github.com/apache/incubator-griffin/pull/463#discussion_r238093471
  
    --- Diff: measure/src/test/scala/org/apache/griffin/measure/utils/HttpUtilTest.scala ---
    @@ -0,0 +1,63 @@
    +/*
    +Licensed to the Apache Software Foundation (ASF) under one
    +or more contributor license agreements.  See the NOTICE file
    +distributed with this work for additional information
    +regarding copyright ownership.  The ASF licenses this file
    +to you under the Apache License, Version 2.0 (the
    +"License"); you may not use this file except in compliance
    +with the License.  You may obtain a copy of the License at
    +
    +  http://www.apache.org/licenses/LICENSE-2.0
    +
    +Unless required by applicable law or agreed to in writing,
    +software distributed under the License is distributed on an
    +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    +KIND, either express or implied.  See the License for the
    +specific language governing permissions and limitations
    +under the License.
    +*/
    +package org.apache.griffin.measure.utils
    +
    +import org.scalatest.{FlatSpec, Matchers}
    +
    +class HttpUtilTest extends FlatSpec with Matchers {
    +  val url = "http://10.148.181.248:39200/griffin/accuracy"
    +  val method = "post"
    +  val user = ""
    +  val password = ""
    +  val params = Map[String, Object]()
    +  val header = Map[String, Object](("Content-Type", "application/json"))
    +  val data = "{}"
    +
    +  "postData httpReq" should "be with auth" in {
    +    try {
    +      HttpUtil.postData(url, user, password, params, header, data)
    +    } catch {
    +      case e: Throwable => println("cannot connect http client." + e)
    +    }
    +  }
    --- End diff --
    
    or something like when... then return..., not familar with scala test.


---

[GitHub] incubator-griffin pull request #463: Dev

Posted by justACT <gi...@git.apache.org>.
Github user justACT commented on a diff in the pull request:

    https://github.com/apache/incubator-griffin/pull/463#discussion_r238091269
  
    --- Diff: measure/src/test/scala/org/apache/griffin/measure/utils/HttpUtilTest.scala ---
    @@ -0,0 +1,63 @@
    +/*
    +Licensed to the Apache Software Foundation (ASF) under one
    +or more contributor license agreements.  See the NOTICE file
    +distributed with this work for additional information
    +regarding copyright ownership.  The ASF licenses this file
    +to you under the Apache License, Version 2.0 (the
    +"License"); you may not use this file except in compliance
    +with the License.  You may obtain a copy of the License at
    +
    +  http://www.apache.org/licenses/LICENSE-2.0
    +
    +Unless required by applicable law or agreed to in writing,
    +software distributed under the License is distributed on an
    +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    +KIND, either express or implied.  See the License for the
    +specific language governing permissions and limitations
    +under the License.
    +*/
    +package org.apache.griffin.measure.utils
    +
    +import org.scalatest.{FlatSpec, Matchers}
    +
    +class HttpUtilTest extends FlatSpec with Matchers {
    +  val url = "http://10.148.181.248:39200/griffin/accuracy"
    +  val method = "post"
    +  val user = ""
    +  val password = ""
    +  val params = Map[String, Object]()
    +  val header = Map[String, Object](("Content-Type", "application/json"))
    +  val data = "{}"
    +
    +  "postData httpReq" should "be with auth" in {
    +    try {
    +      HttpUtil.postData(url, user, password, params, header, data)
    +    } catch {
    +      case e: Throwable => println("cannot connect http client." + e)
    +    }
    +  }
    --- End diff --
    
    but I find if I mock one local variable, I need to use powermock, and it will change pom dependency. 


---

[GitHub] incubator-griffin pull request #463: Dev

Posted by bhlx3lyx7 <gi...@git.apache.org>.
Github user bhlx3lyx7 commented on a diff in the pull request:

    https://github.com/apache/incubator-griffin/pull/463#discussion_r238096832
  
    --- Diff: measure/src/test/scala/org/apache/griffin/measure/utils/HttpUtilTest.scala ---
    @@ -0,0 +1,63 @@
    +/*
    +Licensed to the Apache Software Foundation (ASF) under one
    +or more contributor license agreements.  See the NOTICE file
    +distributed with this work for additional information
    +regarding copyright ownership.  The ASF licenses this file
    +to you under the Apache License, Version 2.0 (the
    +"License"); you may not use this file except in compliance
    +with the License.  You may obtain a copy of the License at
    +
    +  http://www.apache.org/licenses/LICENSE-2.0
    +
    +Unless required by applicable law or agreed to in writing,
    +software distributed under the License is distributed on an
    +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    +KIND, either express or implied.  See the License for the
    +specific language governing permissions and limitations
    +under the License.
    +*/
    +package org.apache.griffin.measure.utils
    +
    +import org.scalatest.{FlatSpec, Matchers}
    +
    +class HttpUtilTest extends FlatSpec with Matchers {
    +  val url = "http://10.148.181.248:39200/griffin/accuracy"
    +  val method = "post"
    +  val user = ""
    +  val password = ""
    +  val params = Map[String, Object]()
    +  val header = Map[String, Object](("Content-Type", "application/json"))
    +  val data = "{}"
    +
    +  "postData httpReq" should "be with auth" in {
    +    try {
    +      HttpUtil.postData(url, user, password, params, header, data)
    +    } catch {
    +      case e: Throwable => println("cannot connect http client." + e)
    +    }
    +  }
    --- End diff --
    
    the pattern is not your limit, we're talking about the goal of UT, just to prove your code could accept "username" and "password" and works well.


---