-
Notifications
You must be signed in to change notification settings - Fork 28
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
Templating for generating random Ports #441
Conversation
@@ -119,13 +120,15 @@ func runTestSpec(t *testing.T, test *TestSpec) (err error) { | |||
startTime := time.Now().UnixNano() / 1000000 | |||
|
|||
for i = -1; i < cmdSpec.Retry; i++ { | |||
err = nil |
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 would be a good spot for a comment
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.
Neha is putting up comments in her next PR.
@@ -148,7 +151,7 @@ func runTestSpec(t *testing.T, test *TestSpec) (err error) { | |||
t.Log("This command :", tmplString, "has re-run", i, "times.") | |||
} | |||
} | |||
return | |||
return err |
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.
Adding err
to return statement is not necessary (see here).
@@ -119,13 +120,15 @@ func runTestSpec(t *testing.T, test *TestSpec) (err error) { | |||
startTime := time.Now().UnixNano() / 1000000 | |||
|
|||
for i = -1; i < cmdSpec.Retry; i++ { | |||
err = nil | |||
|
|||
cmdString := generateCmdString(&cmdSpec) | |||
tmplOutput, tmplErr := performTemplating(strings.Join(cmdString, " "), cache) | |||
if tmplErr != nil { | |||
err = fmt.Errorf("Executing templating failed: %s", tmplErr) |
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.
If there is an error with templating, we don't want to keep retrying (which makes setting err to nil unnecessary).
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 can make the templating error return the same as the timeout err statement, however, the reason the err = nil
is there is for the regex err. If there is a miss matched output and the command is allowed to retry, it should retry that number of times.
@@ -119,13 +120,15 @@ func runTestSpec(t *testing.T, test *TestSpec) (err error) { | |||
startTime := time.Now().UnixNano() / 1000000 |
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 think you can simplify your time code. Just use time.now()
and later check elapsed (eg, elapsed := time.Since(startTime)
). I believe the units you're using for the timeout are seconds (needs a comment!), so multiply the timeout by time.Second so you can compare against elapsed.
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.
See Go Playground for a simple example.
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.
timeout and delay are defined in milliseconds.
@JosephGJ @generalhenry Henry closed this before I finished my review. Please address my comments now as a separate PR. |
creates random port numbers for templating. Similar to random name generation. A range of ports can be supplied to the template.
Added curl method (uncommented) in
service.yml
as retry, timeout and delay fields implemented. Regex moved intolookup.yml
to test
go test github.com/appcelerator/amp/cmd/amp/cli