feat: Introduce new npmExecuteTests step #5124
Conversation
| - PARAMETERS | ||
| - STAGES | ||
| - STEPS | ||
| default: "npm install" |
| - STAGES | ||
| - STEPS | ||
| mandatory: true | ||
| default: "wdi5" |
There was a problem hiding this comment.
Above we expose the tool npm, here we don't. We should be consistent. default: "npm run wdi5"
| type: e2e | ||
| containers: | ||
| - name: node | ||
| image: node:lts-buster |
There was a problem hiding this comment.
buster is a pretty old image. use a more recent one.
vars/npmExecuteTests.groovy
Outdated
| if (config.credentialsId) { | ||
| if (config.wdi5) { | ||
| credentials.add([type: 'usernamePassword', id: config.credentialsId, env: ['wdi5_username', 'wdi5_password']]) | ||
| } else { | ||
| credentials.add([type: 'usernamePassword', id: config.credentialsId, env: ['e2e_username', 'e2e_password']]) | ||
| } | ||
| } |
There was a problem hiding this comment.
We should avoid doing such special handling in the groovy. That way we will have no support for it in GHA / ADO.
There was a problem hiding this comment.
So should we store those in Vault as well?
There was a problem hiding this comment.
Something like:
{
"appSecrets": {
"urls": [
{
"url": "loocalhost:8080",
"username": "some-username1",
"password": "some-password1"
},
{
"url": "loocalhost:8080",
"username": "some-username2",
"password": "some-password2"
}
],
"username": "base-url-username",
"password": "base-url-password"
}
}
| if configOptions.OpenFile == nil { | ||
| return stepConfig, errors.New("config: open file function not set") | ||
| } |
There was a problem hiding this comment.
It is, but it's been missed in some old PR and it's a hard bug to catch, might add it as a new PR
| params: | ||
| - name: installCommand | ||
| type: string | ||
| description: Command to be executed for installation. Defaults to `npm ci`. |
There was a problem hiding this comment.
I think the "defaults to" can be omitted as defaults are printed in the docs.
| - name: onlyRunInProductiveBranch | ||
| type: bool | ||
| default: false | ||
| description: Boolean to indicate whether the step should only be executed in the productive branch or not. | ||
| scope: | ||
| - PARAMETERS | ||
| - STAGES | ||
| - STEPS | ||
| - name: productiveBranch | ||
| type: string | ||
| default: "main" | ||
| description: The branch used as productive branch. | ||
| scope: | ||
| - GENERAL | ||
| - PARAMETERS | ||
| - STAGES | ||
| - STEPS |
There was a problem hiding this comment.
Is that really needed? I would not make a simple step aware of where it's running. This should be part of the pipeline I would say.
| - name: reports | ||
| type: reports | ||
| params: | ||
| - filePattern: "**/e2e-results.xml" |
There was a problem hiding this comment.
Is this a typical file pattern for WDI5?
There was a problem hiding this comment.
I don't think you can provide report with wdi5
| - STEPS | ||
| mandatory: true | ||
| default: "npm run wdi5" | ||
| - name: appSecrets |
There was a problem hiding this comment.
Name is misleading as it also contains the URLs
cmd/npmExecuteTests.go
Outdated
| prefix := "e2e" | ||
| if wdi5 { | ||
| prefix = "wdi5" | ||
| } |
There was a problem hiding this comment.
What about leaving this to the user by exposing a credentialEnvVarPrefix parameter?
cmd/npmExecuteTests.go
Outdated
| log.Entry().Infof("Running end to end tests for URL: %s", url) | ||
|
|
||
| // Execute the npm script | ||
| options := "--baseUrl=" + url |
There was a problem hiding this comment.
I guess this is tool specific and should be configurable or maybe expose as an env var as well?
cmd/npmExecuteTests.go
Outdated
|
|
||
| username := config.AppSecrets["username"].(string) | ||
| password := config.AppSecrets["password"].(string) | ||
| credentialsToEnv(username, password, isWdi5) |
There was a problem hiding this comment.
Are empty credentials working here..?
cmd/npmExecuteTests.go
Outdated
| if ok { | ||
| for _, urlRaw := range urlsRaw { | ||
| urlMap := urlRaw.(map[string]interface{}) | ||
| url := urlMap["url"].(string) | ||
| appURLs[url] = AppURL{ | ||
| URL: url, | ||
| Username: urlMap["username"].(string), | ||
| Password: urlMap["password"].(string), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
We can handle the single url case with the collection as well.
| if ok { | |
| for _, urlRaw := range urlsRaw { | |
| urlMap := urlRaw.(map[string]interface{}) | |
| url := urlMap["url"].(string) | |
| appURLs[url] = AppURL{ | |
| URL: url, | |
| Username: urlMap["username"].(string), | |
| Password: urlMap["password"].(string), | |
| } | |
| } | |
| } | |
| if ok { | |
| for _, urlRaw := range urlsRaw { | |
| urlMap := urlRaw.(map[string]interface{}) | |
| url := urlMap["url"].(string) | |
| appURLs[url] = AppURL{ | |
| URL: url, | |
| Username: urlMap["username"].(string), | |
| Password: urlMap["password"].(string), | |
| } | |
| } | |
| } else { | |
| appURLs[url] = AppURL{ | |
| URL: config.BaseURL, | |
| } | |
| } |
There was a problem hiding this comment.
I think this complicates it a bit
cmd/npmExecuteTests.go
Outdated
| for _, urlRaw := range urlsRaw { | ||
| urlMap := urlRaw.(map[string]interface{}) | ||
| url := urlMap["url"].(string) | ||
| appURLs[url] = AppURL{ |
There was a problem hiding this comment.
The key is not used at all, so this could also be a list of AppUrL.
cmd/npmExecuteTests.go
Outdated
| } | ||
|
|
||
| func runNpmExecuteTests(config *npmExecuteTestsOptions, c command.ExecRunner) error { | ||
| type AppURL struct { |
There was a problem hiding this comment.
| type AppURL struct { | |
| type AppUnderTest struct { |
|
Changes
ADR