-
Notifications
You must be signed in to change notification settings - Fork 209
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
fix: unblocks integration tests by fixing cube properties #381
Conversation
Failing PR (using latest upstream/master) - lordofthejars/swarmq#2 |
32512bf brings a fix to unblock integration tests for boosters in a way they are set up (with dedicated profile). So no need to change anything in the code to make those tests run for a PR build. PR build executes IT test - https://gist.github.com/5894e005ee20b18a939b6e1ea526b37c |
@rupalibehera Can you please expedite this? This is important. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Will let @rupalibehera approve since she understand the code better.
@@ -29,7 +29,7 @@ def call(body) { | |||
echo "WARNING: Integration tests are current DISABLED for these pipelines!" | |||
|
|||
} else { | |||
sh "mvn org.apache.maven.plugins:maven-failsafe-plugin:2.18.1:integration-test ${kubeNS} -Dit.test=${config.itestPattern} -DfailIfNoTests=${config.failIfNoTests} org.apache.maven.plugins:maven-failsafe-plugin:2.18.1:verify" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please shed some light into the implication of removing the version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will use the one user has defined in his build. E.g. latest one is 2.20.1
if I remember correctly and have numerous of fixes. Unless we exactly specify why 2.18.1
is needed I think we should safe users from such surprises.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea (git blame doesn't explain) why the version was pinned in the first place. @bartoszmajsak thanks for the explanation thou.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome. We might think about making it configurable, but only if really necessary. Fixing version and having it potentially different from the user's choice adds another layer of confusion in this lib :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
This fix enables integration tests to be executed during "CI build" (on the PR).
The major issue was using wrong
kubeNS
environment variable to instruct Arquillian Cube to run IT tests of boosters.Tested on OpenShift.io both for PR build and
master
:Important to emphasize here is that Integration Tests (
IT
s) are only executed duringCI
mode of the pipeline library, so when PR is open. Those tests never ran againstmaster
.Fixes openshiftio/openshift.io#2299
Relates to openshiftio/booster-common#8