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

Ci tuning #59

Merged
merged 5 commits into from Mar 8, 2019
Merged

Ci tuning #59

merged 5 commits into from Mar 8, 2019

Conversation

duncdrum
Copy link
Contributor

@duncdrum duncdrum commented Mar 1, 2019

refactored our ci config and test setup.
Build images in parallel, and use bats to execute unit test. The main motivation was the lack of information given the recent breakage, while not much of a speed improvement it also introduces the ability to run tests locally, and should make debugging simpler moving forward.

Currently the test for any Errors in the logs is skipped, we're still not there yet.

@duncdrum duncdrum added the enhancement New feature or request label Mar 1, 2019
@duncdrum duncdrum mentioned this pull request Mar 3, 2019
Copy link
Contributor

@grantmacken grantmacken left a comment

Choose a reason for hiding this comment

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

Some test runners will randomize test sequence order,
So as a rule of thumb, test a, should not depend on something happening in test b.
I am not sure what BATS does but ...
If we test a .. OK we can copy from container
then test b copy into container .. should not depend on the fact there is a copied file
Likewise when we test the ability change password.
If BATS always runs tests in sequence, this won't be a problem otherwise ...

@duncdrum
Copy link
Contributor Author

duncdrum commented Mar 4, 2019

bats always run in sequence, splitting the tests like this makes it easier to see where stuff breaks imv, otherwise we d have multiple steps in one @test (like the teardowns) which tends to hide relevant test output from the logs
also nesting is not an option, which I would have preferred.

@duncdrum duncdrum requested a review from a team March 8, 2019 11:03
Copy link
Contributor

@grantmacken grantmacken left a comment

Choose a reason for hiding this comment

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

@duncdrum
I had another chance to look at this,

  1. I was a bit concerned about ' pipefail' flag not present in BATS,
    but I think the way you have written the specs, this should not be a problem.
  2. I think the .travis.yml file could be improved, something like
jobs:
   include:
     - install 
        - docker build --tag existdb/exist-ci-build:${VERSION} --build-arg "VERSION=${VERSION}" .  
        - docker build --tag existdb/exist-ci-build:${COMMIT} --build-arg "BRANCH=${COMMIT}"
    - before_script
       - docker-compose up -d { VERSION UP port xxx COMMIT UP port yyy same netwok }
    - script
        - { smoke test for COMMIT}
        - { smoke test for VERSON}
   - after_script
       - docker-compose down
   - stage: test
     before_install: docker-compose up -d
     install:
         - sudo add-apt-repository ppa:duggan/bats --yes
         - sudo apt-get update -qq
         - sudo apt-get install -qq bats
         - sleep 20
     script: bats --tap test/*.bats
  - stage: release
  - script:
       - echo "$DOCKER_PASSWORD" | docker login -u "$DOCKER_USERNAME" --password-stdin
       - docker build --tag existdb/exist-ci-build:latest --tag existdb/exist-ci-build:${BRANCH} --build-arg "BRANCH=${BRANCH}" .
       - docker push existdb/exist-ci-build:latest
       - docker push existdb/exist-ci-build:${BRANCH}

this will require a few adjustment to .env and the docker-compose file so..
I think you should merge, what you've got and I'll create branch from the merge and try this out

@duncdrum
Copy link
Contributor Author

duncdrum commented Mar 8, 2019

@grantmacken great, thanks for the review. i m a bit hesitant about using compose for testing as it modifies the containers behaviour, and my primary goal is to make sure that the standalone container is running properly. There is also the fact that docker-compose the library adds another dependency. But I'll gladly take a look at what you come up with.

@duncdrum duncdrum merged commit 6900884 into develop Mar 8, 2019
@duncdrum duncdrum deleted the ci-tuning branch March 12, 2019 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants