Skip to content
This repository has been archived by the owner on Nov 5, 2021. It is now read-only.

Unit tests for service spec creation #34

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MarcPer
Copy link
Contributor

@MarcPer MarcPer commented Jul 9, 2018

Signed-off-by: Marcelo Pereira marcelovitor.pereira@gmail.com

Description

This PR addresses issue #36.

Unit tests were added for checking the generated service spec. A make task was also included for running the tests.

Motivation and Context

This was done to increase test coverage, specifically for the run command.

How Has This Been Tested?

The unit tests were checked with go test locally and on Travis CI. Furthermore, the jaas run command was run with all possible flags and the resulting swarm services were checked.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@MarcPer
Copy link
Contributor Author

MarcPer commented Jul 9, 2018

Sorry for adding a PR that is not finished, but this way it is already clear where I'm going and I can get some feedback before adding more tests. I will also create an issue for this.

Signed-off-by: Marcelo Pereira <marcelovitor.pereira@gmail.com>
Signed-off-by: Marcelo Pereira <marcelovitor.pereira@gmail.com>
@MarcPer MarcPer changed the title WIP: Unit tests for service spec creation Unit tests for service spec creation Jul 14, 2018
@MarcPer
Copy link
Contributor Author

MarcPer commented Jul 14, 2018

All flags that define the created docker service were tested with unit tests.

@alexellis
Copy link
Owner

Hi @MarcPer, sorry that this fell off my radar, I don't even remember seeing notifications for this PR.

It looks useful, but I'll need to find some time to go through this in detail and test it before merging.

Alex

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants