[GitHub] rabbah commented on a change in pull request #3950: Extend system testsuite
rabbah commented on a change in pull request #3950: Extend system testsuite
URL:
https://github.com/apache/incubator-openwhisk/pull/3950#discussion_r227867194
##
File path: tests/src/test/scala/system/basic/WskActionTests.scala
##
@@ -210,6 +253,32 @@ class WskActionTests extends TestHelpers with
WskTestHelpers with JsHelpers with
}
}
+ it should "update an action with different language and check preserving
params" in withAssetCleaner(wskprops) {
+(wp, assetHelper) =>
+ val name = "updatedAction"
+
+ assetHelper.withCleaner(wsk.action, name, false) { (action, _) =>
+wsk.action.create(
+ name,
+ Some(TestUtils.getTestActionFilename("hello.js")),
+ parameters = Map("name" -> testString.toJson)) //unused in the first
function
+ }
+
+ val run1 = wsk.action.invoke(name, Map("payload" -> testString.toJson))
+ withActivation(wsk.activation, run1) { activation =>
+activation.response.status shouldBe "success"
+activation.logs.get.mkString(" ") should include(s"hello, $testString")
+ }
+
+ wsk.action.create(name,
Some(TestUtils.getTestActionFilename("hello.py")), update = true)
+
+ val run2 = wsk.action.invoke(name)
+ withActivation(wsk.activation, run2) { activation =>
+activation.response.status shouldBe "success"
+activation.logs.get.mkString(" ") should include(s"Hello $testString")
+ }
Review comment:
`run2` could just be `run`? minor point.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3950: Extend system testsuite
rabbah commented on a change in pull request #3950: Extend system testsuite
URL:
https://github.com/apache/incubator-openwhisk/pull/3950#discussion_r209907128
##
File path: tests/src/test/scala/system/basic/WskActionTests.scala
##
@@ -210,6 +253,32 @@ class WskActionTests extends TestHelpers with
WskTestHelpers with JsHelpers with
}
}
+ it should "update an action with different language and check preserving
params" in withAssetCleaner(wskprops) {
+(wp, assetHelper) =>
+ val name = "updatedAction"
+
+ assetHelper.withCleaner(wsk.action, name, false) { (action, _) =>
+wsk.action.create(
+ name,
+ Some(TestUtils.getTestActionFilename("hello.js")),
+ parameters = Map("name" -> testString.toJson)) //unused in the first
function
+ }
+
+ val run1 = wsk.action.invoke(name, Map("payload" -> testString.toJson))
+ withActivation(wsk.activation, run1) { activation =>
+activation.response.status shouldBe "success"
+activation.logs.get.mkString(" ") should include(s"hello, $testString")
+ }
+
+ wsk.action.create(name,
Some(TestUtils.getTestActionFilename("hello.py")), update = true)
+
+ val run2 = wsk.action.invoke(name)
+ withActivation(wsk.activation, run2) { activation =>
+activation.response.status shouldBe "success"
+activation.logs.get.mkString(" ") should include(s"Hello $testString")
+ }
Review comment:
fair - I think the api test iirc (didn’t verify again) checks with the
parameters with a get instead of invoke.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3950: Extend system testsuite
rabbah commented on a change in pull request #3950: Extend system testsuite
URL:
https://github.com/apache/incubator-openwhisk/pull/3950#discussion_r209900210
##
File path: tests/src/test/scala/whisk/core/cli/test/WskEntitlementTests.scala
##
@@ -160,6 +160,46 @@ abstract class WskEntitlementTests extends TestHelpers
with WskTestHelpers with
}
}
+ it should "list shared packages when package is turned into public" in
withAssetCleaner(guestWskProps) {
+(wp, assetHelper) =>
+ assetHelper.withCleaner(wsk.pkg, samplePackage) { (pkg, _) =>
+pkg.create(samplePackage)(wp)
+ }
+
+ retry {
+val packageList =
wsk.pkg.list(Some(s"/$guestNamespace"))(defaultWskProps)
+verifyPackageNotSharedList(packageList, guestNamespace, samplePackage)
+ }
+
+ wsk.pkg.create(samplePackage, update = true, shared = Some(true))(wp)
+
+ retry {
+val packageList =
wsk.pkg.list(Some(s"/$guestNamespace"))(defaultWskProps)
+verifyPackageSharedList(packageList, guestNamespace, samplePackage)
+ }
+ }
+
+ it should "not list any packages from invalid namespace" in
withAssetCleaner(guestWskProps) { (wp, assetHelper) =>
+val invalidNamespace = "whisk.systsdf"
+retry {
+ val packageList = wsk.pkg.list(Some(s"/$invalidNamespace"))(wp)
+ packageList.stdout should be("[]") //empty list
+}
+ }
+
+ it should "reject getting package from invalid namespace" in
withAssetCleaner(guestWskProps) { (wp, assetHelper) =>
+val invalidNamespace = "whisk.systsdf"
+wsk.pkg.get(s"/${invalidNamespace}/utils", expectedExitCode =
forbiddenCode)(wp).stderr should include(
+ "not authorized")
+ }
+
+ it should "reject getting invalid package from valid namespace" in
withAssetCleaner(guestWskProps) {
+(wp, assetHelper) =>
+ val invalidPackage = "util"
+ wsk.pkg.get(s"/whisk.system/${invalidPackage}", expectedExitCode =
forbiddenCode)(wp).stderr should include(
+"not authorized")
+ }
Review comment:
👍👍
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3950: Extend system testsuite
rabbah commented on a change in pull request #3950: Extend system testsuite
URL:
https://github.com/apache/incubator-openwhisk/pull/3950#discussion_r20980
##
File path: tests/src/test/scala/common/rest/WskRestOperations.scala
##
@@ -733,13 +738,23 @@ class RestActivationOperations(implicit val actorSystem:
ActorSystem)
fieldFilter: Option[String] = None,
last: Option[Boolean] = None,
summary: Option[Boolean] = None)(implicit wp: WskProps):
RestResult = {
-val rr = activationId match {
+val actId = activationId match {
+ case Some(id) => activationId
+ case None =>
+last match {
Review comment:
The value of the “last” activation might be the last id indexed not
necessarily the last id stored. This is fine for your test related changes from
what I scanned but felt it necessary to mention it as this behavior of last
isn’t generally obvious.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3950: Extend system testsuite
rabbah commented on a change in pull request #3950: Extend system testsuite URL: https://github.com/apache/incubator-openwhisk/pull/3950#discussion_r209899618 ## File path: tests/src/test/scala/system/basic/WskActionTests.scala ## @@ -71,6 +71,21 @@ class WskActionTests extends TestHelpers with WskTestHelpers with JsHelpers with } } + it should "invoke an action that throws an uncaught exception and returns correct status code" in withAssetCleaner( Review comment: That’s fine. There are “basic” action tests in core that do the same. Historically the system package implied tests might be used to check the health of a deployment. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rabbah commented on a change in pull request #3950: Extend system testsuite
rabbah commented on a change in pull request #3950: Extend system testsuite
URL:
https://github.com/apache/incubator-openwhisk/pull/3950#discussion_r209893371
##
File path:
tests/src/test/scala/whisk/core/controller/test/ActivationsApiTests.scala
##
@@ -462,6 +462,57 @@ class ActivationsApiTests extends ControllerTestCommon
with WhiskActivationsApi
}
}
+ it should "skip activations and return correct ones" in {
+implicit val tid = transid()
Review comment:
@chetanmeh do you mind taking a look at these skip related tests as it’s
something you’ve tweaked in the past.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3950: Extend system testsuite
rabbah commented on a change in pull request #3950: Extend system testsuite
URL:
https://github.com/apache/incubator-openwhisk/pull/3950#discussion_r209893171
##
File path: tests/src/test/scala/common/rest/WskRestOperations.scala
##
@@ -733,13 +738,23 @@ class RestActivationOperations(implicit val actorSystem:
ActorSystem)
fieldFilter: Option[String] = None,
last: Option[Boolean] = None,
summary: Option[Boolean] = None)(implicit wp: WskProps):
RestResult = {
-val rr = activationId match {
+val actId = activationId match {
+ case Some(id) => activationId
Review comment:
@markusthoemmes orElse?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3950: Extend system testsuite
rabbah commented on a change in pull request #3950: Extend system testsuite
URL:
https://github.com/apache/incubator-openwhisk/pull/3950#discussion_r209893036
##
File path: tests/src/test/scala/common/rest/WskRestOperations.scala
##
@@ -733,13 +738,23 @@ class RestActivationOperations(implicit val actorSystem:
ActorSystem)
fieldFilter: Option[String] = None,
last: Option[Boolean] = None,
summary: Option[Boolean] = None)(implicit wp: WskProps):
RestResult = {
-val rr = activationId match {
+val actId = activationId match {
+ case Some(id) => activationId
+ case None =>
+last match {
Review comment:
Caution here is that last night be stale.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3950: Extend system testsuite
rabbah commented on a change in pull request #3950: Extend system testsuite
URL:
https://github.com/apache/incubator-openwhisk/pull/3950#discussion_r209892089
##
File path: tests/src/test/scala/whisk/core/cli/test/WskEntitlementTests.scala
##
@@ -160,6 +160,46 @@ abstract class WskEntitlementTests extends TestHelpers
with WskTestHelpers with
}
}
+ it should "list shared packages when package is turned into public" in
withAssetCleaner(guestWskProps) {
+(wp, assetHelper) =>
+ assetHelper.withCleaner(wsk.pkg, samplePackage) { (pkg, _) =>
+pkg.create(samplePackage)(wp)
+ }
+
+ retry {
+val packageList =
wsk.pkg.list(Some(s"/$guestNamespace"))(defaultWskProps)
+verifyPackageNotSharedList(packageList, guestNamespace, samplePackage)
+ }
+
+ wsk.pkg.create(samplePackage, update = true, shared = Some(true))(wp)
+
+ retry {
+val packageList =
wsk.pkg.list(Some(s"/$guestNamespace"))(defaultWskProps)
+verifyPackageSharedList(packageList, guestNamespace, samplePackage)
+ }
+ }
+
+ it should "not list any packages from invalid namespace" in
withAssetCleaner(guestWskProps) { (wp, assetHelper) =>
+val invalidNamespace = "whisk.systsdf"
+retry {
+ val packageList = wsk.pkg.list(Some(s"/$invalidNamespace"))(wp)
+ packageList.stdout should be("[]") //empty list
+}
+ }
+
+ it should "reject getting package from invalid namespace" in
withAssetCleaner(guestWskProps) { (wp, assetHelper) =>
+val invalidNamespace = "whisk.systsdf"
+wsk.pkg.get(s"/${invalidNamespace}/utils", expectedExitCode =
forbiddenCode)(wp).stderr should include(
+ "not authorized")
+ }
+
+ it should "reject getting invalid package from valid namespace" in
withAssetCleaner(guestWskProps) {
+(wp, assetHelper) =>
+ val invalidPackage = "util"
+ wsk.pkg.get(s"/whisk.system/${invalidPackage}", expectedExitCode =
forbiddenCode)(wp).stderr should include(
+"not authorized")
+ }
Review comment:
This suggests that the swagger validation should be added to the unit tests
as well - of which there are many. Is this something we might be able to do?
Otherwise we might end up adding a lot of system level tests that do the same
thing as the unit tests.
I’m not entirely sure we can integrate swagger validation though in the unit
tests - at least not in the way Ben did it for the system tests. But I have not
researched this. I’m mentioning it in case of a black hole here that’s hard to
get out of.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3950: Extend system testsuite
rabbah commented on a change in pull request #3950: Extend system testsuite
URL:
https://github.com/apache/incubator-openwhisk/pull/3950#discussion_r209890859
##
File path: tests/src/test/scala/system/basic/WskActionTests.scala
##
@@ -210,6 +253,32 @@ class WskActionTests extends TestHelpers with
WskTestHelpers with JsHelpers with
}
}
+ it should "update an action with different language and check preserving
params" in withAssetCleaner(wskprops) {
+(wp, assetHelper) =>
+ val name = "updatedAction"
+
+ assetHelper.withCleaner(wsk.action, name, false) { (action, _) =>
+wsk.action.create(
+ name,
+ Some(TestUtils.getTestActionFilename("hello.js")),
+ parameters = Map("name" -> testString.toJson)) //unused in the first
function
+ }
+
+ val run1 = wsk.action.invoke(name, Map("payload" -> testString.toJson))
+ withActivation(wsk.activation, run1) { activation =>
+activation.response.status shouldBe "success"
+activation.logs.get.mkString(" ") should include(s"hello, $testString")
+ }
+
+ wsk.action.create(name,
Some(TestUtils.getTestActionFilename("hello.py")), update = true)
+
+ val run2 = wsk.action.invoke(name)
+ withActivation(wsk.activation, run2) { activation =>
+activation.response.status shouldBe "success"
+activation.logs.get.mkString(" ") should include(s"Hello $testString")
+ }
Review comment:
So here you are testing that the bound parameter is not lost? Did you check
the actions api tests for a comparable test? If there’s one there do we need to
add this? If so, should it also be out of the system suite.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3950: Extend system testsuite
rabbah commented on a change in pull request #3950: Extend system testsuite URL: https://github.com/apache/incubator-openwhisk/pull/3950#discussion_r209888951 ## File path: tests/src/test/scala/common/rest/WskRestOperations.scala ## @@ -707,18 +709,21 @@ class RestActivationOperations(implicit val actorSystem: ActorSystem) * @param entity the name of the entity to filter from activation list * @param limit the maximum number of entities to list (if entity name is not unique use Some(0)) * @param since (optional) only the activations since this timestamp are included + * @param skip (optional) the number of activations to skip * @param retries the maximum retries (total timeout is retries + 1 seconds) * @return activation ids found, caller must check length of sequence */ override def pollFor(N: Int, entity: Option[String], limit: Option[Int] = Some(30), since: Option[Instant] = None, + skip: Option[Int] = Some(0), Review comment: Same q why is default some 0 not none. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rabbah commented on a change in pull request #3950: Extend system testsuite
rabbah commented on a change in pull request #3950: Extend system testsuite URL: https://github.com/apache/incubator-openwhisk/pull/3950#discussion_r209890129 ## File path: tests/src/test/scala/system/basic/WskActionTests.scala ## @@ -71,6 +71,21 @@ class WskActionTests extends TestHelpers with WskTestHelpers with JsHelpers with } } + it should "invoke an action that throws an uncaught exception and returns correct status code" in withAssetCleaner( Review comment: System tests are intended to for sniffing the health of the system. This seems unnecessary here and the test could be moved to the core actions test suite, no? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rabbah commented on a change in pull request #3950: Extend system testsuite
rabbah commented on a change in pull request #3950: Extend system testsuite URL: https://github.com/apache/incubator-openwhisk/pull/3950#discussion_r209888577 ## File path: tests/src/test/scala/common/WskCliOperations.scala ## @@ -480,12 +480,14 @@ class CliActivationOperations(val wsk: RunCliCmd) extends ActivationOperations w * @param filter (optional) if define, must be a simple entity name * @param limit (optional) the maximum number of activation to return * @param since (optional) only the activations since this timestamp are included + * @param skip (optional) the number of activations to skip * @param expectedExitCode (optional) the expected exit code for the command * if the code is anything but DONTCARE_EXIT, assert the code is as expected */ def list(filter: Option[String] = None, limit: Option[Int] = None, since: Option[Instant] = None, + skip: Option[Int] = Some(0), Review comment: Default to none instead of 0? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rabbah commented on a change in pull request #3950: Extend system testsuite
rabbah commented on a change in pull request #3950: Extend system testsuite
URL:
https://github.com/apache/incubator-openwhisk/pull/3950#discussion_r209888233
##
File path: tests/src/test/scala/common/WskOperations.scala
##
@@ -313,6 +313,7 @@ trait ActivationOperations {
entity: Option[String],
limit: Option[Int] = None,
since: Option[Instant] = None,
+ skip: Option[Int] = Some(0),
Review comment:
Why doesn’t this default to none?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3950: Extend system testsuite
rabbah commented on a change in pull request #3950: Extend system testsuite
URL:
https://github.com/apache/incubator-openwhisk/pull/3950#discussion_r209888072
##
File path: tests/src/test/scala/common/WskCliOperations.scala
##
@@ -495,6 +497,10 @@ class CliActivationOperations(val wsk: RunCliCmd) extends
ActivationOperations w
since map { i =>
Seq("--since", i.toEpochMilli.toString)
} getOrElse Seq.empty
+} ++ {
+ since map { i =>
Review comment:
Since? Should this be skip instead.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3950: Extend system testsuite
rabbah commented on a change in pull request #3950: Extend system testsuite URL: https://github.com/apache/incubator-openwhisk/pull/3950#discussion_r208494908 ## File path: tests/dat/actions/argsPrint.js ## @@ -1,6 +1,8 @@ // Licensed to the Apache Software Foundation (ASF) under one or more contributor Review comment: why isn't argsPrint.js its own file leaving applicationError.js alone? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
