[GitHub] [incubator-openwhisk] falkzoll commented on a change in pull request #4450: Fix test cases to also work with nodejs:10 as default nodejs runtime.
falkzoll commented on a change in pull request #4450: Fix test cases to also
work with nodejs:10 as default nodejs runtime.
URL:
https://github.com/apache/incubator-openwhisk/pull/4450#discussion_r277969441
##
File path:
tests/src/test/scala/org/apache/openwhisk/core/limits/ActionLimitsTests.scala
##
@@ -66,7 +66,7 @@ class ActionLimitsTests extends TestHelpers with
WskTestHelpers with WskActorSys
val openFileAction = TestUtils.getTestActionFilename("openFiles.js")
val openFileLimit = 1024
- val minExpectedOpenFiles = openFileLimit - 15 // allow for already opened
files in container
+ val minExpectedOpenFiles = openFileLimit - 20 // allow for already opened
files in container
Review comment:
The nodejs:10 runtime has some more (+5) already open file handles which
reduces the remaining number of open files by 5. The test is `successfully
invoke an action when it is within nofile limit` therefore it seems not really
necessary to always test for the exact maximum of files that can be opened.
With the `-20` the test now runs fine with both versions of the nodejs runtimes.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [incubator-openwhisk] falkzoll commented on a change in pull request #4450: Fix test cases to also work with nodejs:10 as default nodejs runtime.
falkzoll commented on a change in pull request #4450: Fix test cases to also
work with nodejs:10 as default nodejs runtime.
URL:
https://github.com/apache/incubator-openwhisk/pull/4450#discussion_r277966220
##
File path: tests/src/test/scala/invokerShoot/ShootInvokerTests.scala
##
@@ -188,13 +189,13 @@ class ShootInvokerTests extends TestHelpers with
WskTestHelpers with JsHelpers w
JsObject("key" -> JsString("copiedAnnot2"), "value" ->
JsBoolean(false)),
JsObject("key" -> JsString("copiedAnnot1"), "value" ->
JsString("copiedAnnotValue1")),
JsObject("key" -> JsString("origAnnot2"), "value" -> JsBoolean(true)),
-JsObject("key" -> JsString("exec"), "value" -> JsString("nodejs:6")),
+//JsObject("key" -> JsString("exec"), "value" -> JsString(runtime)),
Review comment:
Sure, forgot that line… will be removed.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [incubator-openwhisk] falkzoll commented on a change in pull request #4450: Fix test cases to also work with nodejs:10 as default nodejs runtime.
falkzoll commented on a change in pull request #4450: Fix test cases to also
work with nodejs:10 as default nodejs runtime.
URL:
https://github.com/apache/incubator-openwhisk/pull/4450#discussion_r277965881
##
File path: tests/dat/actions/openFiles.js
##
@@ -22,7 +22,7 @@ function main(params) {
console.log("opened files = ", openFiles.length);
openFiles.forEach(function(fh) {
-fs.close(fh);
+fs.close(fh, (err) => {} );
Review comment:
The empty callback was added since this parameter was optional for nodejs:6
but is now required for nodejs:10. Using an empty callback funtion sounded to
be the default behaviour when this parameter is not present in nodejs:6.
Explicitly defining the empty callback now, works with both nodejs:6 and
nodejs:10.
You are right, making this a `closeSync ` call will most likely not change
the test behaviour, since this is cleanup code at the end of the test. But I
would need to try it first with both node versions.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [incubator-openwhisk] falkzoll commented on a change in pull request #4450: Fix test cases to also work with nodejs:10 as default nodejs runtime.
falkzoll commented on a change in pull request #4450: Fix test cases to also
work with nodejs:10 as default nodejs runtime.
URL:
https://github.com/apache/incubator-openwhisk/pull/4450#discussion_r277528481
##
File path: tests/src/test/scala/invokerShoot/ShootInvokerTests.scala
##
@@ -172,6 +172,7 @@ class ShootInvokerTests extends TestHelpers with
WskTestHelpers with JsHelpers w
it should "add new parameters and annotations while copying an action" in
withAssetCleaner(wskprops) {
(wp, assetHelper) =>
+ val runtime = "nodejs:10"
Review comment:
yes, makes sense, I will look for a generic check like `nodejs:*` and see if
we can modify the tests accordingly … the travis 'System Tests' also still show
a failure I need to analyse and fix… I will get back to you when I am done with
that :-)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [incubator-openwhisk] falkzoll commented on a change in pull request #4450: Fix test cases to also work with nodejs:10 as default nodejs runtime.
falkzoll commented on a change in pull request #4450: Fix test cases to also
work with nodejs:10 as default nodejs runtime.
URL:
https://github.com/apache/incubator-openwhisk/pull/4450#discussion_r276241007
##
File path: tests/src/test/scala/invokerShoot/ShootInvokerTests.scala
##
@@ -172,6 +172,7 @@ class ShootInvokerTests extends TestHelpers with
WskTestHelpers with JsHelpers w
it should "add new parameters and annotations while copying an action" in
withAssetCleaner(wskprops) {
(wp, assetHelper) =>
+ val runtime = "nodejs:10"
Review comment:
Right, that would be the best 👍.
Unfortunately many tests explicitly check the returned runtime string, e.g.
for 'nodejs:6'. When we use 'nodejs:default' at action creation, this returned
runtime string would be whatever the actual default runtime is.
In this case we would need to either query somehow in the test case what the
current default runtime is, which we then can use to compare in the test case.
Or, we could check against a list of possible nodejs runtimes (nodejs:6,
nodejs:8, nodejs:10). But then we would also need to adjust this list when we
add newer versions.
Or, the other option would be, we do not compare the returned runtime string
at all. Which would be the easiest solution,
What do you think?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
