more test coverage for generate/app #13892
Conversation
|
[test] |
pkg/generate/app/app_test.go
Outdated
| @@ -14,6 +14,7 @@ import ( | |||
| imageapi "github.com/openshift/origin/pkg/image/api" | |||
|
|
|||
| _ "github.com/openshift/origin/pkg/api/install" | |||
| "strings" | |||
There was a problem hiding this comment.
This should go up to other core imports.
There was a problem hiding this comment.
I realized I left that in there last night and knew you'd call it out 😄 You're so predictable, @soltysh. I should've left some long ifs in there too.
pkg/generate/app/app_test.go
Outdated
| val string | ||
| expectedReturn bool | ||
| }{ | ||
| {"foo", false}, |
There was a problem hiding this comment.
Add a comment that you're testing parents inside of the for loop, otherwise someone might miss that there.
| {"$foo}", false}, | ||
| {"foo}", false}, | ||
| {"{foo", false}, | ||
| {"${foo", true}, |
There was a problem hiding this comment.
Sigh... this makes me 😢, why we allow this? Is it for supporting spaces inside of those or what? @bparees
There was a problem hiding this comment.
i have no idea. @smarterclayton coded IsParameterizableValue, let's ask him.
but it should not be for supporting spaces, regex would handle that. I think it's just a lazy check.
There was a problem hiding this comment.
So someone can do
oc new-app ruby --name='${name}' -o yaml
And get something they can create a template from
There was a problem hiding this comment.
i don't think that's the question. The question is why the check doesn't look for/require a closing brace.
There was a problem hiding this comment.
That's exactly my question, currently we're only looking for opening bracket but no accompanying closing one.
There was a problem hiding this comment.
I've spawned #13994 to address this.
pkg/generate/app/builder_test.go
Outdated
| "testing" | ||
|
|
||
| imageapi "github.com/openshift/origin/pkg/image/api" | ||
| "reflect" |
There was a problem hiding this comment.
Again, up with the core imports.
| t.Fatalf("failed parsing empty host url: %v", err) | ||
| } | ||
|
|
||
| hostPortURL, err := url.Parse("https://www.example.com:80") |
There was a problem hiding this comment.
how about a non-standard port?
pkg/generate/app/builder_test.go
Outdated
| }, | ||
| expectedReturn: true, | ||
| }, | ||
| "has legacy envrionment STI_LOCATION": { |
pkg/generate/app/builder_test.go
Outdated
| }, | ||
| expectedReturn: true, | ||
| }, | ||
| "has legacy envrionment STI_BUILDER": { |
pkg/generate/app/builder_test.go
Outdated
| }, | ||
| expectedReturn: true, | ||
| }, | ||
| "has legacy envrionment STI_SCRIPTS_URL": { |
|
[test] |
|
LGTM |
|
[test] |
|
re[test] |
1 similar comment
|
re[test] |
|
[test] |
|
Evaluated for origin test up to 1c8795c |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1483/) (Base Commit: b96ef9a) |
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1483/) (Base Commit: b96ef9a) (Image: devenv-rhel7_6232) |
|
Evaluated for origin merge up to 1c8795c |
@soltysh related but does not completely resolve #3016
Added some more tests while I was in some of the code.
app.go 64.9->65.5%
builder.go 0->75.4%
componentref.go 0->12.4%