Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Initial CLI command tests #305

Merged
merged 1 commit into from
Oct 17, 2016
Merged

Initial CLI command tests #305

merged 1 commit into from
Oct 17, 2016

Conversation

JosephGJ
Copy link
Contributor

@JosephGJ JosephGJ commented Oct 12, 2016 •
edited by subfuzion
Loading

Closes #284

To test:

$ go test github.com/appcelerator/amp/cmd/amp/cli

@subfuzion subfuzion self-assigned this Oct 12, 2016
@subfuzion subfuzion changed the title Cli command Tests Initial CLI command tests Oct 12, 2016
@subfuzion
Copy link
Contributor

@JosephGJ I rebased for you and force pushed. To update your branch:

$ git fetch origin cli-command-tests
$ git reset --hard origin/cli-command-tests

@subfuzion
Copy link
Contributor

@JosephGJ I updated the test command for you.

subfuzion
subfuzion reviewed Oct 13, 2016
View reviewed changes
Copy link
Contributor

@subfuzion subfuzion left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests pass. Nice work @JosephGJ. I've got trivial comments for you to address, but overall, great job.

cmd/amp/cli/cli_test.go
"testing"
"time"

// Amplifier
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments removed.

cmd/amp/cli/cli_test.go
"google.golang.org/grpc"
)

type TestSpec struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't export test types in the cli package. Better yet, rename this package to cli_test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test types removed.

cmd/amp/cli/cli_test.go
dockerDefaultVersion = "1.24"
)

// Amplifier
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amplifier set up code not necessary. There is a helper function to start the server here. See TestMain code here for an example. You've already imported the package, so you should be able to just use the helper functions like the example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed unnecessary amplifier setup code. Used the StartTestServer function suggested.

cmd/amp/cli/cli_test.go
}

func TestCmds(t *testing.T) {
parseEnv()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need this (you needed it for TestMain).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed from TestCmds

cmd/amp/cli/cli_test.go
}
commandFinal := append(cmdSplit, spec.Args...)
commandFinal = append(commandFinal, optionsSplit...)
fmt.Println(commandFinal, "Command passed")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use fmt for tests; instead, use t.Log, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t.Log used in every instance of a fmt.Print statement.

cmd/amp/cli/cli_test.go
}

func RunCmd(t *testing.T, cmdString []string) {
fmt.Println(cmdString, "Running...")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same: don't use fmt

cmd/amp/cli/cli_test.go
} else {
fmt.Print(cmdString, " Command result:\n", string(result))
}
time.Sleep(2 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the Sleep call? cmd.CombinedOutput() blocks until finished, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sleep call removed. Replaced with a for loop for recursing a command that fails. Primarly for the purpose of ensure that the pinger isn't curled before pinger has appeared in the swarm.

@JosephGJ
Copy link
Contributor Author

Modified to take into account tony's comments:

  • Removed amplifier setup code
  • fmt.Println replaced with t.Log
  • Sleep call replaced with a for loop designed to recursive over a failing command.
  • Unnecessary comments and types removed

Same test as before:
$ go test github.com/appcelerator/amp/cmd/amp/cli

Will continue to working on Cli command tests.

subfuzion
subfuzion reviewed Oct 16, 2016
View reviewed changes
cmd/amp/cli/cli_test.go
testSpec := &TestSpec{
fileName: name,
contents: contents,
valid: valid,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

$ go test github.com/appcelerator/amp/cmd/amp/cli
# github.com/appcelerator/amp/cmd/amp/cli_test
cmd/amp/cli/cli_test.go:59: undefined: valid
FAIL    github.com/appcelerator/amp/cmd/amp/cli [build failed]

@subfuzion
Copy link
Contributor

Don't merge master (64321d0), please rebase your PRs (and squash your commits) instead.

subfuzion
subfuzion approved these changes Oct 17, 2016
View reviewed changes
Copy link
Contributor

@subfuzion subfuzion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, @JosephGJ 👍

@subfuzion subfuzion merged commit a9b49ea into master Oct 17, 2016
@subfuzion subfuzion deleted the cli-command-tests branch October 17, 2016 18:53
subfuzion pushed a commit that referenced this pull request Oct 17, 2016
@generalhenry @subfuzion
Stack listing options (#331)
6e5733d 
* Initial Cli command Tests (#305)

* Added -l -n and -a to amp stack ls
generalhenry added a commit that referenced this pull request Oct 17, 2016
@generalhenry
improved error messages, noise moved to verbose mode, consistant ids
9ab0edb 
Stack listing options (#331)

* Initial Cli command Tests (#305)

* Added -l -n and -a to amp stack ls

fix merge
@subfuzion subfuzion added this to the 0.2.0 milestone Oct 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@subfuzion subfuzion subfuzion approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.2.0
Development

Successfully merging this pull request may close these issues.
2 participants
@JosephGJ @subfuzion