Added functional tests for CLI start command #3870
Conversation
Signed-off-by: Marian Labuda <[email protected]>
|
Can one of the admins verify this patch? |
TylerJewell
added
the
kind/enhancement
|
Nice improvement. I learned something new by reading this about how to find open ports. |
| docker run -v $BATS_BASE_DIR:$BATS_BASE_DIR -e CLI_IMAGE_TAG=$TAG -e BATS_BASE_DIR=$BATS_BASE_DIR -v /var/run/docker.sock:/var/run/docker.sock $IMAGE_NAME bats $BATS_BASE_DIR/tests/cmd_init_destroy_tests.bats | ||
|
|
||
| echo "Running functionals bats tests for start command" | ||
| docker run -v $BATS_BASE_DIR:$BATS_BASE_DIR -e CLI_IMAGE_TAG=$TAG -e BATS_BASE_DIR=$BATS_BASE_DIR -v /var/run/docker.sock:/var/run/docker.sock $IMAGE_NAME bats $BATS_BASE_DIR/tests/cmd_start_tests.bats |
There was a problem hiding this comment.
question: do you think we could automate the commands ? (using a function)
because most of the lines are copy/paste of "docker run -v $BATS_BASE_DIR:$BATS_BASE_DIR -e CLI_IMAGE_TAG=$TAG -e BATS_BASE_DIR=$BATS_BASE_DIR -v /var/run/docker.sock:/var/run/docker.sock $IMAGE_NAME bats $BATS_"
There was a problem hiding this comment.
Sure thing, I will extract subcommand as a function.
| echo "Removing named container $1" | ||
| docker rm $1 1>/dev/null | ||
| fi | ||
| } | ||
|
|
||
| # Pass a port as an argument to check whether is free or not |
There was a problem hiding this comment.
should these utility methods should be moved in a library_test ?
There was a problem hiding this comment.
I don't think so. The test_base.sh is parent of all tests, if I would move all utility functions to a library_tests.sh, than the test_base.sh would have only few constants in header, sourced library and nothing else (no functions). It would be a transitive sourcing and not needed, I think.
| @test "test cli 'start' with custom port" { | ||
| #GIVEN | ||
| tmp_path=${TESTRUN_DIR}/start2 | ||
| free_port=$(get_free_port) |
There was a problem hiding this comment.
do we need get_free_port method ? AFAIK in docker we're in isolated env
There was a problem hiding this comment.
Yes, we need it. I was looking into Che and it is using default network interface docker0 (On my side 172.17.0.1). Che server runs in a container with an IP (here 172.17.0.4). Che server is accessible via port 8080. I can access to the server via 172.17.0.4:8080 but also via 172.17.0.1:8080. But the server also expose ports and forward ports to localhost. So the server is accessible also on localhost:8080. The port 8080 can be found in state LISTEN in netstat output. Same applies also to workspaces, but those take ports from 32xxx (at least now when I was running them, haven't look closer to scripts what is the algorithm to choose ports). And those are forwarded as well.
Ok now to the reason why I need free ports. First problem could be caused if there would be other docker images running and those would use port required by a che server. It would fail. Second problem is having a service running on localhost (e.g. I have running wildfly locally, which uses port 8080). And this would fail too.
There was a problem hiding this comment.
I still think it is strange. Because the check is performed inside the container while che for example use a docker port mapping.
So I don't see if when we check inside the container we can check the host ports if we don't have a port mapping
There was a problem hiding this comment.
Workspace map ports to free ports, but the che-server which is using this free port function is using same port and if on host is the port used, I cannot start the che server there. Therefore I cannot run che server with CHE_PORT=alreadyusedport and tests would fail.
There was a problem hiding this comment.
Well, I did more digging. The way to get free ports is working on a host, but inside a container it is not working, basically because container itself is isolated from other containers... I will have to find a way to get free ports from inside, at the moment it is not working as expected, unfortunately (it takes first port from the range in free ports function, because output of netstat is empty).
There was a problem hiding this comment.
yes this was my first impression. But it's not a big issue
|
Applied changes to have correctly working netstat in a che-bats container. Basically I had to run the container in host network to have proper data. Now it works. |
|
Added to Release Note - ok for me |
|
While it works on my computer when I tested, it is not on the CI server: #3902 |
slemeur
added
kind/task
* Added functional tests for CLI start command Signed-off-by: Marian Labuda <[email protected]>
Signed-off-by: Marian Labuda [email protected]
What does this PR do?
Added basic functional tests for CLI start command. Plus few minors:
Changelog and Release Note Information
Changelog
Added basic CLI 'start' functional tests
Relase Notes and Docs PR N/A