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

Run tests in a docker container. #39

Merged
merged 9 commits into from
Jan 31, 2020
Merged

Run tests in a docker container. #39

merged 9 commits into from
Jan 31, 2020

Conversation

winder
Copy link
Contributor

@winder winder commented Jan 30, 2020

Run cucumber tests in a docker container.
Make setup.sh more configurable (scripts/docker/setup.py), needs some more work to allow a way to pull down production binaries.

For now you can use run_docker.sh to build/execute the tests. You can also pass flags to test.sh to configure the tests as usual, for example once the image is built run your local code in the container mount your source directory and tell the setup script where to find the mounted directory:

docker run -it \
      -v $(pwd):/opt/sdk-testing \
      -v /home/will/algorand/java-algorand-sdk:/opt/mounted_java_sdk \
      sdk-testing:latest \
      /bin/bash -c "\
            GO111MODULE=off && \
            ./scripts/docker/setup.py --java-dir /opt/mounted_java_sdk && \
            ./scripts/docker/test.sh --java"

ENV DEBIAN_FRONTEND noninteractive

# Basic dependencies
ENV HOME /opt
Copy link
Contributor

Choose a reason for hiding this comment

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

I would leave ENV var settings for the end of the Dockerfile, so that when you decide to change them you don't have to redownload all of your dependencies each time. A good rule of thump is that you want order steps by their likelihood to change, so you can better take advantage of the docker cache

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use $HOME throughout the script so I put this one at the top


rootdir=`dirname $0`
pushd $rootdir
docker build -t sdk-testing -f ./scripts/docker/Dockerfile $rootdir
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better for us to put this in a docker registry than have people build it every time. This will make tests take longer to run and create a blocker when the dockerfile breaks (which it will eventually since people update packages, change urls and stuff)

RUN apt-get update && apt-get install -y apt-utils curl git git-core bsdmainutils

# Install python dependencies
ENV PYENV_ROOT $HOME/pyenv
Copy link
Contributor

@bricerisingalgorand bricerisingalgorand Jan 30, 2020

Choose a reason for hiding this comment

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

This should probably be replaced with something like this like we were talking about in slack

RUN apt-get update && \
    apt-get install -y \
    build-essential \
    libssl-dev \
    libffi-dev \
    python3-dev \
    python3-pip \

Copy link
Contributor Author

Choose a reason for hiding this comment

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

atp-get updat runs a couple lines earlier

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that it could be better to install python3 and then do pip3 installs for any dependencies we need, unless there's a reason why we need to use pyenv

# Install Java dependencies
RUN apt-get install -y maven default-jdk

# Install Go dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use the golang official image as a base instead of installing all this on ubuntu?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that, but you could say the same about java/javascript/python. In terms of complexity python is the most involved installation at the moment.


RUN mkdir -p $HOME/sdk-testing
WORKDIR $HOME/sdk-testing
CMD ["/bin/bash"]
Copy link
Contributor

Choose a reason for hiding this comment

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

why not make this CMD "GO111MODULE=off && ./scripts/setup.sh && ./scripts/test.sh"? That way those that just want to run the test don't need to know the contents of the docker image to run them and just do:

docker run -v $(pwd):/opt/sdk-testing sdk-testing:latest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that idea for the default case and will make the change, but I'm sort of imagining this as a runtime environment. In most cases what is now the test.sh command will have a number of additional options and/or more volumes mounted.

run_docker.sh Outdated
rootdir=`dirname $0`
pushd $rootdir
docker build -t sdk-testing -f ./scripts/docker/Dockerfile $rootdir
docker run -v $(pwd):/opt/sdk-testing sdk-testing:latest /bin/bash -c "GO111MODULE=off && ./scripts/setup.sh && ./scripts/test.sh"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a -ti option to this so that a user can exit the test more easily if they want to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@winder
Copy link
Contributor Author

winder commented Jan 31, 2020

I'm going to merge this in so that I can use it for template testing, it shouldn't have any effect to existing behaviors, we can do that later if we want to make this the preferred way to run the cucumber tests.

@winder winder merged commit dd6278e into master Jan 31, 2020
@winder winder deleted the will/containerize branch January 31, 2020 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants