-
Notifications
You must be signed in to change notification settings - Fork 244
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
update tests so they run on openshift 4.2 #2059
update tests so they run on openshift 4.2 #2059
Conversation
ea40944
to
fbe5866
Compare
/retest
|
fbe5866
to
30a6fe1
Compare
@@ -41,8 +41,8 @@ var _ = Describe("odo storage command", func() { | |||
|
|||
Context("when running storage command without required flag(s)", func() { | |||
It("should fail", func() { | |||
helper.CopyExample(filepath.Join("source", "dotnet"), context) | |||
helper.CmdShouldPass("odo", "component", "create", "dotnet", "dotnet", "--app", "dotnetapp", "--project", project, "--context", context) | |||
helper.CopyExample(filepath.Join("source", "openjdk"), context) |
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.
Need import java IS oc.ImportJavaIs(project)
. Why dotnet component is removed ?
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.
As @amitkrout pointed out, why is the dotnet test replaced with java:8? Also why are we not checking for java imagestream here?
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.
This test is never creating the component. So it doesn't really matter.
DotNet image has a lot of problems, and it is not our primary focus right now.
Purpose of this test is to test url, everything else is just adding time. I will change this to nodejs, so we don't have to import any extra builder images
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.
Hmm, understandable.
tests/integration/java_test.go
Outdated
@@ -73,10 +73,10 @@ var _ = Describe("odoJavaE2e", func() { | |||
}) | |||
|
|||
It("Should be able to deploy a git repo that contains a java uberjar application using openjdk", func() { | |||
oc.ImportJavaIsToNspace(project) | |||
oc.ImportJavaIs(project) |
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 feel ImportJavaIsToNspace
is more talkative.
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.
tests/integration/source_test.go
Outdated
@@ -121,6 +121,8 @@ var _ = Describe("odoSourceE2e", func() { | |||
}) | |||
|
|||
It("Should be able to deploy a dotnet source application", func() { | |||
oc.ImportDotnet20Is(project) |
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.
Now i can understand why you use ImportJavaIs
instead of ImportJavaIsToNspace
.
+1 for ImportDotnet20Is
val, ok := os.LookupEnv("CI") | ||
if ok && val == "openshift" { | ||
// checkForImageStream checks if there is a ImageStram with name and tag in openshift namespace | ||
func (oc *OcRunner) checkForImageStream(name string, tag string) bool { |
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.
Please include the namespace as an argument so that the function can also check the IS in a specified namespace.
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 is not needed, anywhere. The only place where we want to check is openshift
namespace.
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.
ok, if later there will be a need then will expose that argument.
2f6c931
to
ae61757
Compare
/retest
|
/retest
|
/retest
|
/test benchmark |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mik-dass The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
/retest |
/lgtm |
/test benchmark |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
12 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/lgtm |
/retest Please review the full test history for this PR and help us cut down flakes. |
ae61757
to
25e7ed5
Compare
a few changes to tests to make them run on 4.2
fixes #2049
issues were: