jvalue / ods

Open Data Service - Make consuming open data easy, safe, and reliable
GNU Affero General Public License v3.0
36 stars 23 forks source link

Improve test setup and workflows for CDCT #368

Closed felix-oq closed 3 years ago

felix-oq commented 3 years ago

This PR introduces the following changes:

felix-oq commented 3 years ago

@lunedis Could you please have a look at the changes regarding Docker and the CI setup? What are your thoughts on the separation of the base and the build stage and the new docker-compose.test.yml file that makes it possible to run unit tests in Docker without having to lint and build the services?

felix-oq commented 3 years ago

I like the idea, it's just a bit weird that you have to re-build the images for testing them, and then rebuild them again for actual execution. In detail: just calling docker-compose -f docker-compose.yml -f docker-compose.test.yml up notification' results in a "jest not found" error, because it uses the production image without dev dependencies. So you either have to execute build again or call docker-compose -f docker-compose.yml -f docker-compose.test.yml up --build notification'. I am however not sure how that can be solved.

Yes, that is a bit unfortunate, but I can't think of an alternative approach that gives the same flexibility either. Using the suggested approach, unit testing and CDCT can both be executed using the same image. Also, building the image for actual execution involves the build stage that contains unit testing nonetheless. The additional docker-compose.test.yml file just offers developers an optional alternative shortcut to execute tests using Docker without having to wait for the full production build.

Otherwise, developers would have run them without Docker if they wanted to skip linting and building. If that is the desired way of doing it, there would be no need to introduce the docker-compose.test.yml file. CDCT however can't always be executed flawlessly without Docker, which is why I came up with this approach.

Nevertheless this issue, there should be an entry in the readme on how to use docker-compose to test if we decide to go that route.

That would definately be helpful for developers. We could also think about providing additional scripts for unit testing to further simplify calling them.

It is also worth noting that the pact-testing scripts need to be called from the root folder, which was not obvious for me (but seems logical in hindsight).

On my machine, the folder from which the scripts are called is irrelevant. For that purpose I have added the line dir=$(dirname "$0") to the script which determines the location of the script file. Then the paths for the docker-compose files are relative to the location of the script.

lunedis commented 3 years ago

On my machine, the folder from which the scripts are called is irrelevant. For that purpose I have added the line dir=$(dirname "$0") to the script which determines the location of the script file. Then the paths for the docker-compose files are relative to the location of the script.

I get this error:

WARNING: The DOCKER_REGISTRY variable is not set. Defaulting to a blank string.
Building notification
ERROR: Couldn't connect to Docker daemon at http+docker://localhost - is it running?

If it's at a non-standard location, specify the URL with the DOCKER_HOST environment variable.

But maybe that's a problem with my environment.