-
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
Dockerizing integration tests #479
Conversation
e64bab0
to
116c7b0
Compare
422fa51
to
1a6fa74
Compare
@docker service create --network amp-infra --name amp-integration-test --restart-condition none appcelerator/amp-integration-test | ||
@n=""; \ | ||
while [[ $${n} == "" ]] ; do \ | ||
n=`docker ps -qf 'name=amp-integration'`; \ |
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 trick only works in a single node setup. Which is likely fine for now. docker 1.13 will be adding support for docker run within swarm overlay networks which would solve the issue.
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.
Yes you're right. I thought about this as an issue.
I didn't know that docker 1.13 will add this feature, but it will definitely solve the issue.
Maybe we should open an issue to keep track of it.
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.
why don't you use docker service ps amp-integration
? That would not be limited to the local node. You won't get the logs but at least you can loop on it to wait for the exited status.
Another trick would be to force the node on which you execute the task, with a constraint.
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.
@ndegory docker service ps amp-integration-test
returns the task id, and I need the container id to get the logs. Here is how it works currently:
- starts the
amp-integration-test
service - get the container id
docker logs -f $CONTAINER_ID
to get the output of the tests, which is cool because it's blocking and exits when the container exits.- get the exit code of the container and exit with this code in the Makefile to report the test exit status
Hopefully, it's clearer with the explanation.
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.
@bquenin if you need the log then the only solution is to use the constraint:
nodeId=$(docker node ls | grep '*' | awk '{print $1}')
docker service create --constraint "node.id == $nodeId" --network amp-infra --name amp-integration-test --restart-condition none appcelerator/amp-integration-test
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.
You're right, I'll add a node constraint.
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.
Looks great, no troubles running it locally. I needs a write up of the changes for the change log, and the docs need to be updated to reflect it (there are references to localhost: in the docs)
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.
You need to get the latest changes and rebase to the master. Other than the small comments, this looks really good.
test-unit: | ||
@for pkg in $(UNIT_TEST_PACKAGES) ; do \ | ||
go test $$pkg ; \ | ||
done | ||
|
||
test-integration: | ||
@docker service rm amp-integration-test || true |
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 is slightly misleading. Even when the test runs correctly, it returns an error.
Error response from daemon: service amp-integration-test not found
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.
Yeah, I'll change the output message
10bc26f
to
c0d41f9
Compare
@JosephGJ I rebased on the master |
cf37495
to
ebfac96
Compare
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.
LGTM
Related to #410
Dockerizing integration tests.
Verification