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.

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

Cli tests - synchronous testing of platform commands #522

Merged
merged 2 commits into from
Dec 15, 2016

Conversation

JosephGJ
Copy link
Contributor

@JosephGJ JosephGJ commented Dec 5, 2016

Adds tests for the amp platform commands:

  • pull
  • start
  • stop
  • status
    (Monitor can't be tested as it does not return to the cli)

Tests are implemented in the setup and tearDown directory platform.yml files. The setup and tearDown YAML test specs are run synchronously to the samples, with setup being before and tearDown being after. In addition, the docker swarm is initialized in the setup and left in the tearDown.

New field added to the YAML file - run. Boolean which says if a command should or should not be run. By default, the commands in setup and tearDown are not run because they can take upwards of 5 minutes to run.

To test, change the run fields to true in the platform.yml in both setup and tearDown directories. Ensure your platform is stopped and run docker swarm leave --force (Or you can reset the docker client), then run:

go test ./tests/cli

@JosephGJ JosephGJ force-pushed the cli-test-platform-commands branch 8 times, most recently from ac3735a to a2f64a6 Compare December 9, 2016 19:12
Copy link
Contributor

@neha-viswanathan neha-viswanathan left a comment

Choose a reason for hiding this comment

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

The tests are failing -

=== RUN   TestCliCmds
--- FAIL: TestCliCmds (18.26s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x80d3e]

goroutine 5 [running]:
panic(0x18dd40, 0xc42000e060)
        /usr/local/go/src/runtime/panic.go:500 +0x1a1
testing.tRunner.func1(0xc420086180)
        /usr/local/go/src/testing/testing.go:579 +0x25d
panic(0x18dd40, 0xc42000e060)
        /usr/local/go/src/runtime/panic.go:458 +0x243
github.com/appcelerator/amp/tests/cli.runCmdSpec(0xc420086180, 0x1, 0xc4200b6080, 0x11, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        /Users/nviswanathan/dev/src/github.com/appcelerator/amp/tests/cli/cli_test.go:207 +0x95e
github.com/appcelerator/amp/tests/cli.runFrameworkSpec(0xc420086180, 0xc420014960)
        /Users/nviswanathan/dev/src/github.com/appcelerator/amp/tests/cli/cli_test.go:136 +0x172
github.com/appcelerator/amp/tests/cli.runFramework(0xc420086180, 0xc420028098, 0x1, 0x1)
        /Users/nviswanathan/dev/src/github.com/appcelerator/amp/tests/cli/cli_test.go:114 +0x4e
github.com/appcelerator/amp/tests/cli.TestCliCmds(0xc420086180)
        /Users/nviswanathan/dev/src/github.com/appcelerator/amp/tests/cli/cli_test.go:90 +0x571
testing.tRunner(0xc420086180, 0x1d0158)
        /usr/local/go/src/testing/testing.go:610 +0x81
created by testing.(*T).Run
        /usr/local/go/src/testing/testing.go:646 +0x2ec
exit status 2
FAIL    github.com/appcelerator/amp/tests/cli   18.274s

@bquenin
Copy link
Contributor

bquenin commented Dec 9, 2016

Yeah unfortunately, tests are also failing for me:

 bq@home  ~/go/src/github.com/appcelerator/amp   cli-test-platform-commands ± go test ./tests/cli
--- FAIL: TestCliCmds (0.15s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x480f0e]

goroutine 5 [running]:
panic(0x58ff80, 0xc42000a1a0)
        /usr/local/go/src/runtime/panic.go:500 +0x1a1
testing.tRunner.func1(0xc420076180)
        /usr/local/go/src/testing/testing.go:579 +0x25d
panic(0x58ff80, 0xc42000a1a0)
        /usr/local/go/src/runtime/panic.go:458 +0x243
github.com/appcelerator/amp/tests/cli.runCmdSpec(0xc420076180, 0x1, 0xc4200b6100, 0x11, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        /home/bq/go/src/github.com/appcelerator/amp/tests/cli/cli_test.go:207 +0x95e
github.com/appcelerator/amp/tests/cli.runFrameworkSpec(0xc420076180, 0xc420014a50)
        /home/bq/go/src/github.com/appcelerator/amp/tests/cli/cli_test.go:136 +0x172
github.com/appcelerator/amp/tests/cli.runFramework(0xc420076180, 0xc420032098, 0x1, 0x1)
        /home/bq/go/src/github.com/appcelerator/amp/tests/cli/cli_test.go:114 +0x4e
github.com/appcelerator/amp/tests/cli.TestCliCmds(0xc420076180)
        /home/bq/go/src/github.com/appcelerator/amp/tests/cli/cli_test.go:90 +0x571
testing.tRunner(0xc420076180, 0x5d2da8)
        /usr/local/go/src/testing/testing.go:610 +0x81
created by testing.(*T).Run
        /usr/local/go/src/testing/testing.go:646 +0x2ec
FAIL    github.com/appcelerator/amp/tests/cli   0.150s

@bquenin
Copy link
Contributor

bquenin commented Dec 9, 2016

Also, and maybe I'm a bit harsh, but I don't really think we need these tests because Travis already pull and start the platform.
It leaves us with amp pf status and amp pf stop.
Maybe you could add an amp pf status test in the CLI tests assuming the platform is already started (by travis)
We could also add an amp pf stop to .travis.yml and make sure it returns no error to automate the process.
Please everyone comment, I'd like to have your point of view on the matter.
Thanks!

@JosephGJ JosephGJ force-pushed the cli-test-platform-commands branch 2 times, most recently from 1cb7832 to ba8fe2d Compare December 9, 2016 23:38
@JosephGJ
Copy link
Contributor Author

JosephGJ commented Dec 9, 2016

I've made a minor change. The tests were failing due to .Error() call in the mismatched expectation error return. In addition, I made the amp pf pull regex more forgiving as image versions are going to get updated repeatedly.

I like your idea @bquenin . Adding the amp pf status and amp pf stop to the travis build would be a good idea. The reason these commands were created was to dockerize the cli tests, and not just dockerize them but run them in docker-in-docker for complete isolation. However, due to some issues we had with running it, the idea was dropped.

I still think the setup and teardown directories for the cli_tests, along with the general restructuring of cli_test.go and the addition of the run field in the yaml files are useful additions to the cli tests, but the platform yaml files can definitely be removed if they aren't necessary.

Copy link
Contributor

@ndegory ndegory left a comment

Choose a reason for hiding this comment

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

change request on test description format

@@ -1,4 +1,5 @@
- logs-all:
run: true
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have a default value for run (true)? this way we only have to specify this key when we want to disable a test.

Copy link
Contributor Author

@JosephGJ JosephGJ Dec 12, 2016

Choose a reason for hiding this comment

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

@ndegory changed to a skip field, so the default value is false and you can give true to skip said command

@bquenin
Copy link
Contributor

bquenin commented Dec 12, 2016

@JosephGJ Do you want me to remove the amp pf start and stop tests and only keep the amp pf statu test in the cli tests ?
Or do you want to do it ?

@JosephGJ JosephGJ force-pushed the cli-test-platform-commands branch from ba8fe2d to d7ffabc Compare December 12, 2016 23:14
@JosephGJ JosephGJ force-pushed the cli-test-platform-commands branch from d7ffabc to e9b27d3 Compare December 12, 2016 23:16
@JosephGJ
Copy link
Contributor Author

@bquenin I've removed the amp pf start and stop commands, as well as any other unnecessary command tests.

Copy link
Contributor

@bquenin bquenin left a comment

Choose a reason for hiding this comment

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

LGTM, tested on linux

@bquenin bquenin merged commit 819f9ff into master Dec 15, 2016
@bquenin bquenin deleted the cli-test-platform-commands branch December 15, 2016 00:16
@JosephGJ JosephGJ added this to the 0.4.0 milestone Dec 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants