Skip to content

Conversation

@rabbah
Copy link
Member

@rabbah rabbah commented Jul 4, 2018

Additional cleanup of tests:

  1. removes WskRest* where possible and makes the base test suite the Rest tests
  2. removes runtime tests should should be moved to their respective runtimes
  3. runs the unicode tests over all the non-deprecated kinds per the runtime manifest

Also fixes #3845. See issue for details.

Description

Related issue and scope

My changes affect the following components

  • API
  • Controller
  • Message Bus (e.g., Kafka)
  • Loadbalancer
  • Invoker
  • Intrinsic actions (e.g., sequences, conductors)
  • Data stores (e.g., CouchDB)
  • Tests
  • Deployment
  • CLI
  • General tooling
  • Documentation

Types of changes

  • Bug fix (generally a non-breaking change which closes an issue).
  • Enhancement or new feature (adds new functionality).
  • Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

  • I signed an Apache CLA.
  • I reviewed the style guides and followed the recommendations (Travis CI will check :).
  • I added tests to cover my changes.
  • My changes require further changes to the documentation.
  • I updated the documentation where necessary.

@chetanmeh
Copy link
Member

@rabbah Do the unicode test pass for you on Mac?. For me they always failed and only passed on travis. May be related to default encoding and we may need to make it more explicit

@rabbah
Copy link
Member Author

rabbah commented Jul 4, 2018

They do yes - do you run the tests from gradle or ij?

@rabbah
Copy link
Member Author

rabbah commented Jul 4, 2018

I just need a sniff tests - it doesn't have to be unicode (which should be covered by the unit proxy tests anyway). So it could be "echo" as well.

/**
* Test the Java "hello world" demo sequence
*/
it should "Invoke a java action" in withAssetCleaner(wskprops) { (wp, assetHelper) =>
Copy link
Member Author

Choose a reason for hiding this comment

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

this test can be dropped.

/*
* Example from the docs.
*/
it should "Invoke a Java action where main is in the default package" in withAssetCleaner(wskprops) {
Copy link
Member Author

@rabbah rabbah Jul 6, 2018

Choose a reason for hiding this comment

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

move this to unit test.

EDIT: added to java runtime tests.

}
}

it should "Ensure that Java actions cannot be created without a specified main method" in withAssetCleaner(wskprops) {
Copy link
Member Author

@rabbah rabbah Jul 6, 2018

Choose a reason for hiding this comment

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

move this to cli/unit test.

EDIT: moved.


behavior of s"Runtime $currentNodeJsKind"

it should "Ensure that NodeJS actions can have a non-default entrypoint" in withAssetCleaner(wskprops) {
Copy link
Member Author

Choose a reason for hiding this comment

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

drop, duplicate of unit test.

}
}

it should "Ensure that zipped actions are encoded and uploaded as NodeJS actions" in withAssetCleaner(wskprops) {
Copy link
Member Author

@rabbah rabbah Jul 6, 2018

Choose a reason for hiding this comment

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

drop, duplicate of unit test.

EDIT: moved to CLI test suite.

}
}

it should "invoke an action and confirm expected environment is defined" in withAssetCleaner(wskprops) {
Copy link
Member Author

Choose a reason for hiding this comment

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

drop, duplicate.

}
}

it should "invoke an invalid action and get error back" in withAssetCleaner(wskprops) { (wp, assetHelper) =>
Copy link
Member Author

@rabbah rabbah Jul 6, 2018

Choose a reason for hiding this comment

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

duplicate - but unit test doesn't check error message as done here, should tighten unit test accordingly.

EDIT: updated unit tests for runtimes accordingly.


behavior of "Python virtualenv"

Seq(("python2_virtualenv.zip", "python:2"), ("python3_virtualenv.zip", "python:3")).foreach {
Copy link
Member Author

Choose a reason for hiding this comment

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

drop, duplicates.


behavior of "Swift runtime"

it should "Ensure that Swift actions can have a non-default entrypoint" in withAssetCleaner(wskprops) {
Copy link
Member Author

Choose a reason for hiding this comment

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

drop, duplicate.

class WskRestActionTests extends WskActionTests with WskActorSystem {
override val wsk = new WskRestOperations

it should "create an action with an empty file" in withAssetCleaner(wskprops) { (wp, assetHelper) =>
Copy link
Member Author

@rabbah rabbah Jul 6, 2018

Choose a reason for hiding this comment

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

The docker skeleton and python runtimes accept an empty code string during init, and fail in run (docker skeleton will just run the stub, and python will return error = action did not return dictionary).

Java will fail with class/method not found for main during class loader.

Swift will fail with did not generate an executable.

Dropping this test is OK - I don't think we're getting any increased coverage. It would be nice to make the failures uniform but this may be a little difficult for all the runtimes to normalize.

See #3839 and #3844.

EDIT: I added tests for empty code strings to the runtime suite.

@rabbah rabbah force-pushed the tests branch 2 times, most recently from ee8be72 to 7c660ae Compare July 6, 2018 03:06
@rabbah rabbah added testing review Review for this PR has been requested and yet needs to be done. labels Jul 6, 2018
@rabbah rabbah force-pushed the tests branch 2 times, most recently from 0968438 to d38af10 Compare July 6, 2018 18:13

val bodyHead = Map("name" -> name.toJson, "namespace" -> namespace.toJson)

val (updateExistence, resultExistence) = update match {
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this because this is mimicking CLI functionality and I think should be considered invalid for the REST operations. If this was an SDK I can see supporting this but here the tests should be making pass through calls and not really doing anything fancy. I kept copy support although I was tempted to remove that also.

}
}

it should "reject action update for sequence with no components" in withAssetCleaner(wskprops) { (wp, assetHelper) =>
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to a unit test.

}
}

it should "report error when creating an action with unknown kind" in withAssetCleaner(wskprops) {
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to a unit test.

rr.stderr should include("kind 'foobar' not in Set")
}

it should "report error when creating an action with zip but without kind" in withAssetCleaner(wskprops) {
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to a unit test.

}
}

it should "ensure action update with --web flag only copies existing annotations when new annotations are not provided" in withAssetCleaner(
Copy link
Member Author

@rabbah rabbah Jul 6, 2018

Choose a reason for hiding this comment

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

this is a CLI test, does not belong with the REST tests.

EDIT: already exists in the cli tests.

@codecov-io
Copy link

codecov-io commented Jul 6, 2018

Codecov Report

Merging #3840 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3840      +/-   ##
==========================================
+ Coverage    74.6%   74.61%   +0.01%     
==========================================
  Files         138      138              
  Lines        6465     6465              
  Branches      406      406              
==========================================
+ Hits         4823     4824       +1     
+ Misses       1642     1641       -1
Impacted Files Coverage Δ
.../scala/src/main/scala/whisk/core/entity/Exec.scala 83.76% <0%> (+0.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c05de4...d2926f7. Read the comment docs.

@dgrove-oss
Copy link
Member

dgrove-oss commented Jul 6, 2018

PG1 3103 🔴

@rabbah rabbah force-pushed the tests branch 5 times, most recently from a6c27dc to a71efae Compare July 7, 2018 16:22
@rabbah rabbah force-pushed the tests branch 3 times, most recently from 7ac0094 to 9e470e3 Compare July 8, 2018 01:01
@markusthoemmes
Copy link
Contributor

Thanks a big bunch for all of this grunt work 👍

Tweak some tests checking status codes.
Fix bug in test which permitted one too many log sentinels.
Add test for non-main entry point
Add one more tests to runtime docs.
rabbah added a commit to rabbah/incubator-openwhisk-cli that referenced this pull request Jul 9, 2018
@rabbah
Copy link
Member Author

rabbah commented Jul 9, 2018

@dgrove-oss All the tests which needed to move have no been moved. This PR is ready. Linked all related PRs:

apache/openwhisk-cli#350
apache/openwhisk-runtime-docker#52 (and git tag as 1.3.2)
apache/openwhisk-runtime-swift#72 (and git tag)
apache/openwhisk-runtime-python#33 (and git tag)
apache/openwhisk-runtime-nodejs#71 (and git tag)
apache/openwhisk-runtime-java#63 (and git tag)
apache/openwhisk-runtime-php#35 (and git tag)

Copy link
Member

@dgrove-oss dgrove-oss left a comment

Choose a reason for hiding this comment

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

looks good to go. I will merge this and then work on merging through all the downstream PRs.

@dgrove-oss dgrove-oss merged commit 5b13c4c into apache:master Jul 9, 2018
dgrove-oss pushed a commit to apache/openwhisk-cli that referenced this pull request Jul 9, 2018
BillZong pushed a commit to BillZong/openwhisk that referenced this pull request Nov 18, 2019
…test for all runtimes (apache#3840)

Additional cleanup of tests:
    removes WskRest* where possible and makes the base test suite the Rest tests
    removes runtime tests should should be moved to their respective runtimes
    runs the unicode tests over all the non-deprecated kinds per the runtime manifest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review Review for this PR has been requested and yet needs to be done. testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WskRestOperation does not properly set the exec kind even when one is specified

5 participants