[GitHub] dubee commented on a change in pull request #3503: move CLI node tests to CLI repo and sync node default tests
dubee commented on a change in pull request #3503: move CLI node tests to CLI repo and sync node default tests URL: https://github.com/apache/incubator-openwhisk/pull/3503#discussion_r179218616 ## File path: tests/src/test/scala/system/basic/WskSequenceTests.scala ## @@ -56,7 +56,7 @@ abstract class WskSequenceTests extends TestHelpers with ScalatestRouteTest with val allowedActionDuration = 120 seconds val shortDuration = 10 seconds - val whiskConfig = new WhiskConfig(Map(WhiskConfig.actionSequenceMaxLimit -> null)) + val whiskConfig: WhiskConfig Review comment: That will fix the test failure Travis is complaining about. 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] dubee commented on a change in pull request #3503: move CLI node tests to CLI repo and sync node default tests
dubee commented on a change in pull request #3503: move CLI node tests to CLI repo and sync node default tests URL: https://github.com/apache/incubator-openwhisk/pull/3503#discussion_r179218148 ## File path: tests/src/test/scala/system/basic/WskSequenceTests.scala ## @@ -56,7 +56,7 @@ abstract class WskSequenceTests extends TestHelpers with ScalatestRouteTest with val allowedActionDuration = 120 seconds val shortDuration = 10 seconds - val whiskConfig = new WhiskConfig(Map(WhiskConfig.actionSequenceMaxLimit -> null)) + val whiskConfig: WhiskConfig Review comment: Remove `assert(whiskConfig.isValid)` here. That can be put into `WskRestSequenceTests.scala`. 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] dubee commented on a change in pull request #3503: move CLI node tests to CLI repo and sync node default tests
dubee commented on a change in pull request #3503: move CLI node tests to CLI
repo and sync node default tests
URL:
https://github.com/apache/incubator-openwhisk/pull/3503#discussion_r178657257
##
File path: tests/src/test/scala/system/basic/WskRuleTests.scala
##
@@ -102,21 +103,28 @@ abstract class WskRuleTests extends TestHelpers with
WskTestHelpers {
case (trigger, status) =>
if (status == active) wsk.rule.enable(ruleName) else
wsk.rule.disable(ruleName)
-// Needs to be retried since the enable/disable causes a cache
invalidation which needs to propagate first
-retry(
- {
-wsk.rule
- .create(ruleName, trigger, actionName, update = true)
- .stdout
- .parseJson
- .asJsObject
- .fields
- .get("status") shouldBe status
-
-
wsk.rule.get(ruleName).stdout.parseJson.asJsObject.fields.get("status")
shouldBe status
- },
- 10,
- Some(1.second))
+if (cli) {
Review comment:
As part of future work, the base tests shouldn't know or care whether the
the REST or CLI is being used. We should be able to provide a special function
to strip out the CLI header that has the same interface that the REST tests use
to prevent adding this special logic in the base tests. Maybe we can override
RunResult for the CLI tests with a method that removes the header.
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] dubee commented on a change in pull request #3503: move CLI node tests to CLI repo and sync node default tests
dubee commented on a change in pull request #3503: move CLI node tests to CLI
repo and sync node default tests
URL:
https://github.com/apache/incubator-openwhisk/pull/3503#discussion_r178657257
##
File path: tests/src/test/scala/system/basic/WskRuleTests.scala
##
@@ -102,21 +103,28 @@ abstract class WskRuleTests extends TestHelpers with
WskTestHelpers {
case (trigger, status) =>
if (status == active) wsk.rule.enable(ruleName) else
wsk.rule.disable(ruleName)
-// Needs to be retried since the enable/disable causes a cache
invalidation which needs to propagate first
-retry(
- {
-wsk.rule
- .create(ruleName, trigger, actionName, update = true)
- .stdout
- .parseJson
- .asJsObject
- .fields
- .get("status") shouldBe status
-
-
wsk.rule.get(ruleName).stdout.parseJson.asJsObject.fields.get("status")
shouldBe status
- },
- 10,
- Some(1.second))
+if (cli) {
Review comment:
As part of future work, the base tests shouldn't know or care whether the
the REST or CLI is being used. We should be able to provide a special function
to strip out the CLI header that has the same interface that the REST tests use
to prevent adding this special logic in the base tests. Maybe we can override
RunResult for the CLI tests that has a method that removes the header.
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] dubee commented on a change in pull request #3503: move CLI node tests to CLI repo and sync node default tests
dubee commented on a change in pull request #3503: move CLI node tests to CLI
repo and sync node default tests
URL:
https://github.com/apache/incubator-openwhisk/pull/3503#discussion_r178657576
##
File path: tests/src/test/scala/system/basic/WskRuleTests.scala
##
@@ -102,21 +103,28 @@ abstract class WskRuleTests extends TestHelpers with
WskTestHelpers {
case (trigger, status) =>
if (status == active) wsk.rule.enable(ruleName) else
wsk.rule.disable(ruleName)
-// Needs to be retried since the enable/disable causes a cache
invalidation which needs to propagate first
-retry(
- {
-wsk.rule
- .create(ruleName, trigger, actionName, update = true)
- .stdout
- .parseJson
- .asJsObject
- .fields
- .get("status") shouldBe status
-
-
wsk.rule.get(ruleName).stdout.parseJson.asJsObject.fields.get("status")
shouldBe status
- },
- 10,
- Some(1.second))
+if (cli) {
Review comment:
Also, as part of future work, we should be able to move `Wsk.scala` to
incubator-openwhisk-cli.
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] dubee commented on a change in pull request #3503: move CLI node tests to CLI repo and sync node default tests
dubee commented on a change in pull request #3503: move CLI node tests to CLI
repo and sync node default tests
URL:
https://github.com/apache/incubator-openwhisk/pull/3503#discussion_r178657257
##
File path: tests/src/test/scala/system/basic/WskRuleTests.scala
##
@@ -102,21 +103,28 @@ abstract class WskRuleTests extends TestHelpers with
WskTestHelpers {
case (trigger, status) =>
if (status == active) wsk.rule.enable(ruleName) else
wsk.rule.disable(ruleName)
-// Needs to be retried since the enable/disable causes a cache
invalidation which needs to propagate first
-retry(
- {
-wsk.rule
- .create(ruleName, trigger, actionName, update = true)
- .stdout
- .parseJson
- .asJsObject
- .fields
- .get("status") shouldBe status
-
-
wsk.rule.get(ruleName).stdout.parseJson.asJsObject.fields.get("status")
shouldBe status
- },
- 10,
- Some(1.second))
+if (cli) {
Review comment:
As part of future work, the base tests shouldn't know or care whether the
the REST or CLI is being used. We should be able to provide a special function
to strip out the CLI header that have the same interface that the REST tests
use to prevent adding this special logic in the base tests. Maybe we can
override RunResult for the CLI tests that has a method that removes the header.
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] dubee commented on a change in pull request #3503: move CLI node tests to CLI repo and sync node default tests
dubee commented on a change in pull request #3503: move CLI node tests to CLI
repo and sync node default tests
URL:
https://github.com/apache/incubator-openwhisk/pull/3503#discussion_r178657168
##
File path: tests/src/test/scala/system/basic/WskRuleTests.scala
##
@@ -102,21 +103,28 @@ abstract class WskRuleTests extends TestHelpers with
WskTestHelpers {
case (trigger, status) =>
if (status == active) wsk.rule.enable(ruleName) else
wsk.rule.disable(ruleName)
-// Needs to be retried since the enable/disable causes a cache
invalidation which needs to propagate first
-retry(
- {
-wsk.rule
- .create(ruleName, trigger, actionName, update = true)
- .stdout
- .parseJson
- .asJsObject
- .fields
- .get("status") shouldBe status
-
-
wsk.rule.get(ruleName).stdout.parseJson.asJsObject.fields.get("status")
shouldBe status
- },
- 10,
- Some(1.second))
+if (cli) {
Review comment:
As part of future work, the base tests shouldn't know or care whether the
the REST or CLI is being used. We should be able to provide a special function
to strip out the CLI header that have the same interface that the REST tests
use to prevent adding this special logic in the base tests. Maybe we can
override `RunResult` for the CLI tests that has a method that removes the
header.
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] dubee commented on a change in pull request #3503: move CLI node tests to CLI repo and sync node default tests
dubee commented on a change in pull request #3503: move CLI node tests to CLI
repo and sync node default tests
URL:
https://github.com/apache/incubator-openwhisk/pull/3503#discussion_r178657168
##
File path: tests/src/test/scala/system/basic/WskRuleTests.scala
##
@@ -102,21 +103,28 @@ abstract class WskRuleTests extends TestHelpers with
WskTestHelpers {
case (trigger, status) =>
if (status == active) wsk.rule.enable(ruleName) else
wsk.rule.disable(ruleName)
-// Needs to be retried since the enable/disable causes a cache
invalidation which needs to propagate first
-retry(
- {
-wsk.rule
- .create(ruleName, trigger, actionName, update = true)
- .stdout
- .parseJson
- .asJsObject
- .fields
- .get("status") shouldBe status
-
-
wsk.rule.get(ruleName).stdout.parseJson.asJsObject.fields.get("status")
shouldBe status
- },
- 10,
- Some(1.second))
+if (cli) {
Review comment:
As part of future work, the base tests shouldn't know or care whether the
the REST or CLI is being used. We should be able to provide a special function
to strip out the CLI header that have the same interface that the REST tests
use to prevent adding this special logic in the base tests. Maybe we can
override `RunResult` for the CLI tests that has a method that removes the
header.
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] dubee commented on a change in pull request #3503: move CLI node tests to CLI repo and sync node default tests
dubee commented on a change in pull request #3503: move CLI node tests to CLI
repo and sync node default tests
URL:
https://github.com/apache/incubator-openwhisk/pull/3503#discussion_r178654286
##
File path: tests/src/test/scala/system/basic/WskRuleTests.scala
##
@@ -102,21 +103,28 @@ abstract class WskRuleTests extends TestHelpers with
WskTestHelpers {
case (trigger, status) =>
if (status == active) wsk.rule.enable(ruleName) else
wsk.rule.disable(ruleName)
-// Needs to be retried since the enable/disable causes a cache
invalidation which needs to propagate first
-retry(
- {
-wsk.rule
- .create(ruleName, trigger, actionName, update = true)
- .stdout
- .parseJson
- .asJsObject
- .fields
- .get("status") shouldBe status
-
-
wsk.rule.get(ruleName).stdout.parseJson.asJsObject.fields.get("status")
shouldBe status
- },
- 10,
- Some(1.second))
+if (cli) {
Review comment:
There is a function that does this. Checkout
https://github.com/apache/incubator-openwhisk/blob/f7efbb6cf09563009b71486158b9232433439660/tests/src/test/scala/common/WskTestHelpers.scala#L282.
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] dubee commented on a change in pull request #3503: move CLI node tests to CLI repo and sync node default tests
dubee commented on a change in pull request #3503: move CLI node tests to CLI
repo and sync node default tests
URL:
https://github.com/apache/incubator-openwhisk/pull/3503#discussion_r178654286
##
File path: tests/src/test/scala/system/basic/WskRuleTests.scala
##
@@ -102,21 +103,28 @@ abstract class WskRuleTests extends TestHelpers with
WskTestHelpers {
case (trigger, status) =>
if (status == active) wsk.rule.enable(ruleName) else
wsk.rule.disable(ruleName)
-// Needs to be retried since the enable/disable causes a cache
invalidation which needs to propagate first
-retry(
- {
-wsk.rule
- .create(ruleName, trigger, actionName, update = true)
- .stdout
- .parseJson
- .asJsObject
- .fields
- .get("status") shouldBe status
-
-
wsk.rule.get(ruleName).stdout.parseJson.asJsObject.fields.get("status")
shouldBe status
- },
- 10,
- Some(1.second))
+if (cli) {
Review comment:
There is a function that does this. Check out
https://github.com/apache/incubator-openwhisk/blob/f7efbb6cf09563009b71486158b9232433439660/tests/src/test/scala/common/WskTestHelpers.scala#L282.
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] dubee commented on a change in pull request #3503: move CLI node tests to CLI repo and sync node default tests
dubee commented on a change in pull request #3503: move CLI node tests to CLI
repo and sync node default tests
URL:
https://github.com/apache/incubator-openwhisk/pull/3503#discussion_r178652735
##
File path: tests/src/test/scala/system/basic/WskRuleTests.scala
##
@@ -91,7 +92,7 @@ abstract class WskRuleTests extends TestHelpers with
WskTestHelpers {
val active = Some("active".toJson)
val inactive = Some("inactive".toJson)
val statusPermutations =
- Seq((triggerName, active), (triggerName, inactive), (triggerName2,
active), (triggerName, inactive))
+ Seq((triggerName, active), (triggerName, inactive), (triggerName2,
active), (triggerName2, inactive))
Review comment:
Nope needs to be `triggerName` otherwise it's just running the first two
permutations again.
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] dubee commented on a change in pull request #3503: move CLI node tests to CLI repo and sync node default tests
dubee commented on a change in pull request #3503: move CLI node tests to CLI
repo and sync node default tests
URL:
https://github.com/apache/incubator-openwhisk/pull/3503#discussion_r178366038
##
File path: tests/src/test/scala/system/basic/WskRuleTests.scala
##
@@ -83,43 +81,6 @@ abstract class WskRuleTests extends TestHelpers with
WskTestHelpers {
behavior of "Whisk rules"
- it should "preserve rule status when a rule is updated" in
withAssetCleaner(wskprops) { (wp, assetHelper) =>
Review comment:
Why does this test need to be moved?
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
