Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the fundamental framework of REST invocation for test cases #2589

Merged
merged 6 commits into from Oct 20, 2017

Conversation

houshengbo
Copy link

@houshengbo houshengbo commented Aug 8, 2017

Almost all the test cases in OpenWhisk are currently running based
on the wsk CLI binary. In order to separate the CLI out of OpenWhisk
core repository, we need to call REST API of OpenWhisk to access
OpenWhisk services instead of calling the wsk CLI binary command.

This PR adds the basic REST implementation for all the test cases
to use. One basic principle is to keep the changes to the existing
test cases as few as possible, so we add WskRest.scala as an equivalent
class to Wsk.scala and WskRestTestHelpers.scala as an equivalent file
to WskTestHelpers.scala for REST.

All the replacement of binary with REST will happen in an increamental fashion.
This PR only changes one existing test case in WskActionTests.scala, and
reimplements it in WskRestActionTests.scala. All the other test cases can follow
the same way to change the test cases one by one. New changes may be necessary
to the basic REST implementation as well in future to accommodate the test cases.

Partially-closes-bug: #2430

@markusthoemmes
Copy link
Contributor

Close #2480 ?

@houshengbo houshengbo force-pushed the basic-rest-framework branch 2 times, most recently from 7fe59ff to 9fc73ff Compare August 8, 2017 19:38
@houshengbo houshengbo changed the title Add the fundamental framework of REST invocation for test cases to re… Add the fundamental framework of REST invocation for test cases Aug 9, 2017
@houshengbo houshengbo force-pushed the basic-rest-framework branch 13 times, most recently from f6df6f4 to 3935ef1 Compare August 15, 2017 17:03
@houshengbo houshengbo added the cli label Aug 15, 2017
@houshengbo houshengbo requested a review from rabbah August 15, 2017 17:11
@houshengbo houshengbo force-pushed the basic-rest-framework branch 8 times, most recently from 38ece33 to 1fb024d Compare August 18, 2017 18:55
(ns, entityName)
}

def invokeAction(name: String,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function can be named more appropriately as it looks like invokeAction will handle all CRUD requests for actions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

invokeaction only invokes actions by POST the existing action. This name should be fine.

@houshengbo houshengbo force-pushed the basic-rest-framework branch 2 times, most recently from 5b8c5e3 to 84f9a6b Compare October 19, 2017 14:48

var (params, annos) = getParamsAnnos(parameters, annotations, parameterFile, annotationFile, web = web)

val inputKindType = kind map { k =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't need this as kind is defined as a Some or None.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

k
} getOrElse null

inputKindType match {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Match on Some or None here for kind. Ex:

kind match {
	case Some(k) if k == "copy" => copy routine
	case Some(k) if k == "sequence" => sequence routine
	case _ => kind is None
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

f
} getOrElse null

if ((feed == null) || (result.statusCode != OK)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would feed be null here if it is a Some or None?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be feedInput, but I will change part anyway.

else requestEntity(PUT, path, body = bodyContent.toString)
val result = new RestResult(resp.status, getRespData(resp))

val feedInput = feed map { f =>
Copy link
Member

@dubee dubee Oct 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't need this, just match on feed like example I left above.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

docs: Boolean = true,
expectedExitCode: Int = SUCCESS_EXIT)(implicit wp: WskProps): RestResult = {
val entityPath = Path(s"${basePath}/namespaces/${wp.namespace}/$noun")
var paramMap = Map("skip" -> "0".toString, "docs" -> docs.toString) ++ {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"0" is already a string. Shouldn't need to use toString there.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@houshengbo houshengbo force-pushed the basic-rest-framework branch 2 times, most recently from 1204724 to e089886 Compare October 19, 2017 16:09
nameSort: Option[Boolean] = None,
expectedExitCode: Int = OK.intValue)(implicit wp: WskProps): RestResult = {
val (ns, name) = getNamespaceActionName(resolve(namespace))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newline here needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.


def waitForActivationConsole(totalWait: Duration = 30 seconds, sinceTime: Instant)(
implicit wp: WskProps): RestResult = {
var result = new RestResult(NotFound, null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible to get rid of null on these new RestResult calls?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@houshengbo houshengbo force-pushed the basic-rest-framework branch 2 times, most recently from 49f4cbf to 8499491 Compare October 19, 2017 17:27
expectedExitCode: Int = SUCCESS_EXIT)(implicit wp: WskProps): RestResult = {
val path = getNamePath(noun, name)
val annos = convertMapIntoKeyValue(annotations)
val published = shared map { s =>
Copy link
Member

@dubee dubee Oct 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you can do shared.getOrElse(false).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

var bodyContent = JsObject("name" -> name.toJson, "namespace" -> s"$ns".toJson)

if (!update) {
val published = shared map { s =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you can do shared.getOrElse(false).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


val (params, annos) = this.getParamsAnnos(parameters, annotations, parameterFile, annotationFile)
if (!update) {
val published = shared map { s =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you can do shared.getOrElse(false).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

private def extractActivationIdFromInvoke(result: RestResult): Option[String] = {
Try {
val activationID =
if ((result.statusCode == OK) || (result.statusCode == Accepted)) result.getField("activationId") else ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it looks like the an empty string returns Some(). I think None should be returned instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should only need this in the function:

Try {
     if ((result.statusCode == OK) || (result.statusCode == Accepted)) Some(result.getField("activationId")) else None
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove try {}, or it will report error.

update: Boolean = false,
expectedExitCode: Int = OK.intValue)(implicit wp: WskProps): RestResult = {

val (ns, triggerName) = this.getNamespaceActionName(name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

triggerName != an action name.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed the function into getNamespaceEntityName, cos it is more appropriate.

val params = convertMapIntoKeyValue(parameters)
val annos = convertMapIntoKeyValue(annotations)

val (ns, packageName) = this.getNamespaceActionName(provider)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

packageName != action name.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed the function into getNamespaceEntityName, cos it is more appropriate.

}
}

def getNamespaceActionName(name: String)(implicit wp: WskProps): (String, String) = {
Copy link
Member

@dubee dubee Oct 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this equivalent?

def getNamespaceAndEntityName(name: String)(implicit wp: WskProps): (Option[String], Option[String]) = {
	name.split("/") match {
		case Array(empty, namespace, entityName) if empty.isEmpty => (Some(namespace), Some(entityName))
		case Array(empty, namespace) if empty.isEmpty => (Some(namespace), None)
		case Array(entityName) => (Some(wp.namespace), Some(entityName))
		case _ => (None, None)
	}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe throw an error on _ case. What do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me see how to rewrite it.
There is no need to do case _ => (None, None), or even return errors, because the worst case is to return wp.namespace and name.

// Invoke the feed
val (nsFeed, feedName) = this.getNamespaceActionName(f)
val path = Path(s"$basePath/namespaces/$nsFeed/actions/$feedName")
val paramMap = Map("blocking" -> "true".toString, "result" -> "false".toString)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both toString calls are not needed here.

@dubee dubee self-assigned this Oct 20, 2017
Vincent Hou added 5 commits October 20, 2017 09:53
…place wsk binary

Almost all the test cases in OpenWhisk are currently running based
on the wsk CLI binary. In order to separate the CLI out of OpenWhisk
core repository, we need to call REST API of OpenWhisk to access
OpenWhisk services instead of calling the wsk CLI binary command.

This PR adds the basic REST implementation for all the test cases
to use. One basic principle is to keep the changes to the existing
test cases as few as possible, so we add WskRest.scala as an equivalent
class to Wsk.scala and WskRestTestHelpers.scala as an equivalent file
to WskTestHelpers.scala for REST.

All the replacement of binary with REST will happen in an increamental fashion.
This PR only changes one existing test case in WskActionTests.scala, and
reimplements it in WskRestActionTests.scala. All the other test cases can follow
the same way to change the test cases one by one. New changes may be necessary
to the basic REST implementation as well in future to accommodate the test cases.
@dubee dubee merged commit 1c5c78b into apache:master Oct 20, 2017
houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Nov 13, 2017
…he#2589)

* Add the fundamental framework of REST invocation for test cases to replace wsk binary

Almost all the test cases in OpenWhisk are currently running based
on the wsk CLI binary. In order to separate the CLI out of OpenWhisk
core repository, we need to call REST API of OpenWhisk to access
OpenWhisk services instead of calling the wsk CLI binary command.

This PR adds the basic REST implementation for all the test cases
to use. One basic principle is to keep the changes to the existing
test cases as few as possible, so we add WskRest.scala as an equivalent
class to Wsk.scala and WskRestTestHelpers.scala as an equivalent file
to WskTestHelpers.scala for REST.

All the replacement of binary with REST will happen in an increamental fashion.
This PR only changes one existing test case in WskActionTests.scala, and
reimplements it in WskRestActionTests.scala. All the other test cases can follow
the same way to change the test cases one by one. New changes may be necessary
to the basic REST implementation as well in future to accommodate the test cases.

* Fixed the concerns regarding the comments

* Fixed comments about null and option match

* Get rid of null for rest result

* Consolidate the option map by getOrElse

* Refactor the function getNamespaceEntityName
houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Nov 13, 2017
…he#2589)

* Add the fundamental framework of REST invocation for test cases to replace wsk binary

Almost all the test cases in OpenWhisk are currently running based
on the wsk CLI binary. In order to separate the CLI out of OpenWhisk
core repository, we need to call REST API of OpenWhisk to access
OpenWhisk services instead of calling the wsk CLI binary command.

This PR adds the basic REST implementation for all the test cases
to use. One basic principle is to keep the changes to the existing
test cases as few as possible, so we add WskRest.scala as an equivalent
class to Wsk.scala and WskRestTestHelpers.scala as an equivalent file
to WskTestHelpers.scala for REST.

All the replacement of binary with REST will happen in an increamental fashion.
This PR only changes one existing test case in WskActionTests.scala, and
reimplements it in WskRestActionTests.scala. All the other test cases can follow
the same way to change the test cases one by one. New changes may be necessary
to the basic REST implementation as well in future to accommodate the test cases.

* Fixed the concerns regarding the comments

* Fixed comments about null and option match

* Get rid of null for rest result

* Consolidate the option map by getOrElse

* Refactor the function getNamespaceEntityName
houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Nov 14, 2017
…he#2589)

* Add the fundamental framework of REST invocation for test cases to replace wsk binary

Almost all the test cases in OpenWhisk are currently running based
on the wsk CLI binary. In order to separate the CLI out of OpenWhisk
core repository, we need to call REST API of OpenWhisk to access
OpenWhisk services instead of calling the wsk CLI binary command.

This PR adds the basic REST implementation for all the test cases
to use. One basic principle is to keep the changes to the existing
test cases as few as possible, so we add WskRest.scala as an equivalent
class to Wsk.scala and WskRestTestHelpers.scala as an equivalent file
to WskTestHelpers.scala for REST.

All the replacement of binary with REST will happen in an increamental fashion.
This PR only changes one existing test case in WskActionTests.scala, and
reimplements it in WskRestActionTests.scala. All the other test cases can follow
the same way to change the test cases one by one. New changes may be necessary
to the basic REST implementation as well in future to accommodate the test cases.

* Fixed the concerns regarding the comments

* Fixed comments about null and option match

* Get rid of null for rest result

* Consolidate the option map by getOrElse

* Refactor the function getNamespaceEntityName
houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Nov 17, 2017
…he#2589)

* Add the fundamental framework of REST invocation for test cases to replace wsk binary

Almost all the test cases in OpenWhisk are currently running based
on the wsk CLI binary. In order to separate the CLI out of OpenWhisk
core repository, we need to call REST API of OpenWhisk to access
OpenWhisk services instead of calling the wsk CLI binary command.

This PR adds the basic REST implementation for all the test cases
to use. One basic principle is to keep the changes to the existing
test cases as few as possible, so we add WskRest.scala as an equivalent
class to Wsk.scala and WskRestTestHelpers.scala as an equivalent file
to WskTestHelpers.scala for REST.

All the replacement of binary with REST will happen in an increamental fashion.
This PR only changes one existing test case in WskActionTests.scala, and
reimplements it in WskRestActionTests.scala. All the other test cases can follow
the same way to change the test cases one by one. New changes may be necessary
to the basic REST implementation as well in future to accommodate the test cases.

* Fixed the concerns regarding the comments

* Fixed comments about null and option match

* Get rid of null for rest result

* Consolidate the option map by getOrElse

* Refactor the function getNamespaceEntityName
BillZong pushed a commit to BillZong/openwhisk that referenced this pull request Nov 18, 2019
…he#2589)

* Add the fundamental framework of REST invocation for test cases to replace wsk binary

Almost all the test cases in OpenWhisk are currently running based
on the wsk CLI binary. In order to separate the CLI out of OpenWhisk
core repository, we need to call REST API of OpenWhisk to access
OpenWhisk services instead of calling the wsk CLI binary command.

This PR adds the basic REST implementation for all the test cases
to use. One basic principle is to keep the changes to the existing
test cases as few as possible, so we add WskRest.scala as an equivalent
class to Wsk.scala and WskRestTestHelpers.scala as an equivalent file
to WskTestHelpers.scala for REST.

All the replacement of binary with REST will happen in an increamental fashion.
This PR only changes one existing test case in WskActionTests.scala, and
reimplements it in WskRestActionTests.scala. All the other test cases can follow
the same way to change the test cases one by one. New changes may be necessary
to the basic REST implementation as well in future to accommodate the test cases.

* Fixed the concerns regarding the comments

* Fixed comments about null and option match

* Get rid of null for rest result

* Consolidate the option map by getOrElse

* Refactor the function getNamespaceEntityName
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli review Review for this PR has been requested and yet needs to be done.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants