Allow overriding mvn commands in integration test #420
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hrishin
changed the title
hrishin
changed the title
|
@bartoszmajsak this would work for you? |
|
@hrishin In theory it might, but in reality not quite. In booster pipelines (but I also think in general use case) we call fabric8-pipeline-library/vars/mavenCI.groovy Lines 65 to 69 in 9dfcb32 |
|
Then how we can provide the interface from Jenkins file? |
|
I think we need to be more specific with the arguments. I could think about following mavenCI {
deploy = 'mvn clean deploy -P openshift'
integrationTests = 'mvn clean install-Dnamespace.use.current=false -DenableImageStreamDetection=true -P openshift-it'
}then in the mavenIntegrationTest {
environment = 'Test'
integrationTests = config.integrationTests
} then the follow up question is what do we do with other "hardcoded bits in there, namely and alikes. |
sthaha
reviewed
Aug 13, 2018
vars/mavenIntegrationTest.groovy
Outdated
| @@ -11,6 +11,8 @@ def call(body) { | |||
| def utils = new Utils() | |||
| def envName = config.environment | |||
| def kubeNS = "-Dfabric8.environment=${envName}" | |||
| def cmd = cmd ?: "mvn org.apache.maven.plugins:maven-failsafe-plugin:integration-test ${kubeNS} -P openshift-it -Dit.test=${config.itestPattern} -DfailIfNoTests=${config.failIfNoTests} org.apache.maven.plugins:maven-failsafe-plugin:verify" | |||
There was a problem hiding this comment.
shouldn't this be config.cmd ?: ...
|
@bartoszmajsak apart from |
|
With this patch now mavenCI {
integrationTestCmd = 'mvn -P openshift-it org.apache.maven.plugins:maven-failsafe-plugin:verify'
} |
|
I have tested this patch. Jenkins logs for integration test. Appears like due to permission issue this test case got failed in order to create new project. |
|
@hrishin your parameters are not enough. Test wants to create namespace You can see the full command in my PR #412 |
hrishin
force-pushed
the
maven-it-fix
branch
11 times, most recently
from
255d559 to
56264e1
Compare
|
Thanks @bartoszmajsak. @sthaha @bartoszmajsak could you review this patch? |
sthaha
reviewed
Aug 14, 2018
vars/mavenCI.groovy
Outdated
| @@ -61,8 +61,16 @@ def call(body) { | |||
| } | |||
| } | |||
|
|
|||
| def testCmd = config.integrationTestCmd | |||
| if(config.runTestsInUserNamespace == true && testCmd) { | |||
There was a problem hiding this comment.
== trueis superfluous and can be deleted
if ( config.runTestsInUserNamespace && testCmd) {
}nit: please leave a space after keywords
There was a problem hiding this comment.
In if (config.runTestsInUserNamespace), if runTestsInUserNamespace has any value (non empty) then it would eveluate the if block. Hence to force boolean contract it's comparing it with true or false.
Does that sounds right?
sthaha
reviewed
Aug 14, 2018
vars/mavenCI.groovy
Outdated
| def testCmd = config.integrationTestCmd | ||
| if(config.runTestsInUserNamespace == true && testCmd) { | ||
| def nameSpace = utils.getUsersNamespace() | ||
| echo "Running the integration tests in the namespace: ${nameSpace}" |
There was a problem hiding this comment.
nameSpace -> namespace
and we don't need "the" in "... in the namespace: ${namespace}"
sthaha
suggested changes
Aug 14, 2018
vars/mavenCI.groovy
Outdated
| if(config.runTestsInUserNamespace == true && testCmd) { | ||
| def nameSpace = utils.getUsersNamespace() | ||
| echo "Running the integration tests in the namespace: ${nameSpace}" | ||
| testCmd = testCmd + " -Dnamespace.use.existing=" + nameSpace |
There was a problem hiding this comment.
can't this be handled in the mavenIntegrationTest instead?
mavenIntegrationTest {
namespace = "foobar"
}If user sets a testCmd, wouldn't they expect to run exactly that?
There was a problem hiding this comment.
If we only talk about using this library in OSIO Build/Jenkins - then yes. Otherwise - why should I need that?
This patch allows the `mavenCI{}` to over maven command in order
to allow `mavenIntegrationTes{}` to pass specific command to execute.
It has added a util method to retrive default test namespace.
Fixes: openshiftio/openshift.io#763
Fixes: openshiftio/openshift.io#3134
Related to: fabric8io#412
|
@sthaha @bartoszmajsak I've updated the patch could you please review it? Jenkinsfiles : here Test results: |
sthaha
reviewed
Aug 23, 2018
vars/mavenCI.groovy
Outdated
| @@ -63,6 +63,7 @@ def call(body) { | |||
|
|
|||
| stage('Integration Testing') { | |||
| mavenIntegrationTest { | |||
| integrationTestCmd = config.integrationTestCmd | |||
There was a problem hiding this comment.
this should just be called cmd instead of integrationTestCmd since we have already established that the current context is mavenIntegrationTest
sthaha
reviewed
Aug 23, 2018
| @@ -2,35 +2,37 @@ | |||
| import io.fabric8.Utils | |||
|
|
|||
| def call(body) { | |||
There was a problem hiding this comment.
How about this
def mvnTestCommandForConfig(config) {
def utils = new Utils()
def envName = config.environment
def kubeNS = "-Dfabric8.environment=${envName}"
if (envName) {
try {
def ns = utils.environmentNamespace(envName)
if (ns) {
kubeNS = "-Dnamespace.use.existing=${ns}"
echo "Running the integration tests in namespace : ${ns}"
}
} catch (e) {
echo "ERROR: failed to find the environment namespace for ${envName} due to ${e}"
e.printStackTrace()
}
}
return "mvn \
org.apache.maven.plugins:maven-failsafe-plugin:integration-test \
-P openshift-it ${kubeNS} \
-Dit.test=${config.itestPattern} \
-DfailIfNoTests=${config.failIfNoTests} \
org.apache.maven.plugins:maven-failsafe-plugin:verify"
}
def call(body) {
def utils = new Utils()
if (utils.isDisabledITests()) {
echo "WARNING: Integration tests DISABLED for these pipelines!"
return
}
// evaluate the body block, and collect configuration into the object
def config = [:]
body.resolveStrategy = Closure.DELEGATE_FIRST
body.delegate = config
body()
// execute integration test command if it is explicitly set and exit
def cmd = "" config.cmd ?: mvnTestCommandForConfig(config)
sh cmd
junitResults(body);
}
There was a problem hiding this comment.
I haven't tested this but the idea is quite simple ... The call either makes the cmd to run or uses the cmd passed to it and it is quite clear IMHO
There was a problem hiding this comment.
this looks more readable +1
sthaha
suggested changes
Aug 23, 2018
|
@sthaha I have updated the patch. Could you please review it? Jenkinsfile: Test results: |
sthaha
approved these changes
Aug 24, 2018
sthaha
suggested changes
Aug 24, 2018
ReadMe.md
Outdated
| - pass `cmd` parameter to override the `mvn` command to execute in integration test | ||
| ```groovy | ||
| mavenIntegrationTest { | ||
| integrationTestCmd = 'mvn -P openshift-it org.apache.maven.plugins:maven-failsafe-plugin:verify' |
There was a problem hiding this comment.
this would now be cmd - right?
- extracted default maven command code to separate method - changed variable names - updated readme
sthaha
approved these changes
Aug 24, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch allows the
mavenCI{}to over maven command in orderto allow
mavenIntegrationTes{}to pass specific command to execute.Related to: #412