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

Dockerised SyTest #484

Merged
merged 5 commits into from
Aug 15, 2018
Merged

Dockerised SyTest #484

merged 5 commits into from
Aug 15, 2018

Conversation

hawkowl
Copy link
Contributor

@hawkowl hawkowl commented Aug 8, 2018

No description provided.

@hawkowl hawkowl requested a review from a team August 8, 2018 11:00
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

some suggestions which might stem entirely from my lack of familiarity with docker, or which might be useful.

I'm not sure if we've got the sytest/synapse divide quite right, but let's ignore that for now.

ENV LANGUAGE en_US:en
ENV LC_ALL en_US.UTF-8

# The dockerfile context, when ran by the buildscript, will actually be the
Copy link
Member

Choose a reason for hiding this comment

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

"when run by"


echo "Trying to get same-named sytest..."
wget -q https://github.com/matrix-org/sytest/archive/$branch_name.tar.gz -O sytest.tar.gz
4
Copy link
Member

Choose a reason for hiding this comment

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

4? I raise you 5.

docker/README.md Outdated

Alternatively:
Once pulled from Docker Hub, the container can be ran on a Synapse checkout:
Copy link
Member

Choose a reason for hiding this comment

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

"can be run on"

```
./run-tests.pl
```
- matrixdotorg/sytest, a base container with SyTest dependencies installed
Copy link
Member

Choose a reason for hiding this comment

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

one thing that I didn't quite grok at first was that the docker images don't in fact include Sytest itself, which will be pulled from git. I realise it's alluded to below, but it might be worth making explicit?

docker/README.md Outdated
Where `<command>` is `./run-tests.pl` or similar.
This will run on the same branch in SyTest as the Synapse checkout, if possible, but will fall back to using develop.

If you want to use a checkout of SyTest, mount it to `/test` inside the container by adding `-v /path/to/sytest\:/test` to the docker command.
Copy link
Member

Choose a reason for hiding this comment

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

s/a checkout/an existing checkout/ ?

@@ -0,0 +1,59 @@
#! /usr/bin/env bash

set -x
Copy link
Member

Choose a reason for hiding this comment

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

looks like we're going to need set -e here too.

Copy link
Member

Choose a reason for hiding this comment

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

(with some faffery around the wget)


# The dockerfile context, when ran by the buildscript, will actually be the
# repo root, not the docker folder
ADD install-deps.pl ./install-deps.pl
Copy link
Member

Choose a reason for hiding this comment

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

having added these to the docker image, are they ever actually used?


set -x

if [ -e "/test/run-tests.pl" ]
Copy link
Member

Choose a reason for hiding this comment

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

why is this this /test/run-tests.pl rather than ./run-tests.pl ? The inconsistency makes me wonder if I'm misunderstanding what is going on. Which is entirely plausible, given my level of docker fu.

.gitignore Outdated
@@ -5,3 +5,7 @@
/.synapse-base
/synapse
/var
*~
\#*
logs/
Copy link
Member

Choose a reason for hiding this comment

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

can we make these /logs and /results.tap rather than matching those filenames anywhere?

.gitignore Outdated
@@ -5,3 +5,7 @@
/.synapse-base
/synapse
/var
*~
Copy link
Member

Choose a reason for hiding this comment

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

there's some attempt to sort this file; this and the below probably want to go alongside .*.swp

@hawkowl hawkowl merged commit 59b5c98 into develop Aug 15, 2018
@hawkowl hawkowl deleted the hawkowl/dockerised-sytest branch August 15, 2018 09:57
@richvdh richvdh mentioned this pull request Sep 18, 2018
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