Remove actionControllers tests from incubator-openwhisk repo #3467
Conversation
|
We will need to designate a single place as the canonical root for the common code. Maybe it can still be this repo. |
|
Maybe it can be, but should we move it into a different namespace from the runtime-specific code (maybe also out of the tests directory since it will at that point be more of a library)? That would reduce the likelihood of collisions and make it clear when a given runtime has migrated to the new paradigm (whatever that may be). |
jonpspri
changed the title
|
Marking as WIP because apparently, the what’s in the runtime repo and what’s in the core repo was even more messy than I originally anticipated. Currently moving in the direction of “Make the runtimes self-sufficient for |
jonpspri
changed the title
|
Removing the WIP tag, because I’ve resolved the namespace oversights as I’ve moved through the runtimes. It’s necessary to go through the tests in individual runtimes and make sure all are namespaced to A transition period will be needed to remove deploy builds from the Travis files, after which the shared classes can be factored back into |
blame @csantanapr 😅 |
|
@jonpspri can you clarify: where will the canonical common code be located after this patch? |
|
Common test code should live in this repo for now. I started refactoring the runtimes repo to not have a copy of this code and leverage the common-test jar produced by maven and installed locally. |
csantanapr
left a comment
csantanapr
left a comment
There was a problem hiding this comment.
Common test code lives in this repo for now and gets compiled into OpenWhisk-common-tests
|
To be precise, these are the modules of concern (all from tests/src/test/scala/actionContainers):
I think there's a path to moving them into one of the shared libraries, but it will require a little extra juggling with all the other changes impact them. Let me think about it for a bit. Also, I'm a bit confused. I see 'openwhisk-common' and 'openwhisk-tests' in the maven local repositories. Are you suggesting another JAR 'openwhisk-common-tests' or a namespace in one of the existing JARs? |
|
Naming WIP again. I intend to find a home for at least |
|
@csantanapr @rabbah Please review. Moving ActionContainer and ResourceHelpers into a 'common.actionContainers' package in the test library seemed to me to be the least disruptive course toward the shared library we're all looking for. Now each runtime will need to 'import common.actionContainers._' and remove the redundant scala from the local repo. Thoughts? |
| testLargeInput(stdLargeInputSamples) | ||
| } | ||
|
|
||
| trait BasicActionRunnerTests extends ActionProxyContainerTestUtils { |
There was a problem hiding this comment.
@jonpspri can you moe this trait to the common package also - this is a boilerplate to run a few standard tests as shown just above on lines 243-247.
|
actually I think tests/src/test/scala/actionContainers/ActionProxyContainerTests.scala can be made more generic and moved to the common package. The idea being that if one wants to test a specific runtime, they can specify the docker image name and some code samples, leaving the rest of the suite the same. I can work with you seperately on making this trait the base of all the other runtime tests. |
|
Yeah, I ran across the trait in question in a couple places during my works on the runtimes. For the moment, I'll factor it over into ActionContainers.scala, which is where the other traits used by the tests live. I also want to break the hard dependency on |
|
Also tinkering with how the Docker URL is discovered, because the newer runtime build Gradles know their Docker hosts and can provide them as part of the environment. |
Codecov Report
@@ Coverage Diff @@
## master #3467 +/- ##
==========================================
+ Coverage 74.11% 74.66% +0.54%
==========================================
Files 127 127
Lines 6043 6051 +8
Branches 388 388
==========================================
+ Hits 4479 4518 +39
+ Misses 1564 1533 -31
Continue to review full report at Codecov.
|
|
@csantanapr I rebased this PR and resolved the conflict. I believe the latest changes address your concerns as well. |
rabbah
added
testing
reviewed
csantanapr
left a comment
csantanapr
left a comment
There was a problem hiding this comment.
don't want to be force to have a whisk.properties file
| Option(WhiskProperties.getProperty("whisk.version.name")).filter(_.toLowerCase.contains("mac")).map { | ||
| case _ => s"tcp://${WhiskProperties.getMainDockerEndpoint}" | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't like this, I don't want the runtimes test to rely on WhiskProperties to have a whisk.properties file present.
If user is using docker-machine the DOCKER_HOST env variable needs to be set and use.
There was a problem hiding this comment.
it's a nice convenience - the precedence order is -D, ENV, then whisk props.
|
PG 5 374 👍 |
|
With this change travis build time for unit test now reduces to 18 mins (from 24 mins) of which test run is only ~6 mins!
|
5 participants
Description
When the various action controllers were spun out into their own repositories, they took their test suites with them. However, the original source code for the test suites was left in
incubator-openwhisk. That was fine until we started compiling JAR files to support testing and leaving them in local maven. Now the JAR files including the actionController tests can create classpath conflicts with the test being compiled from the individual runtimes, challenging the validity of the tests.This PR deletes the not-canonical versions of the tests from incubator-runtime. There are certain utility classes that are duplicated among the runtime repos that should maybe eventually be factored further into an SDK, but for the moment simplicity in the test process is needed to ensure test validity.
Related issue and scope
My changes affect the following components
Types of changes
Checklist: