Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

tests: added tests for all commands except run, write, copy, and version #55

Merged
merged 1 commit into from
Oct 30, 2015

Conversation

cgonyeo
Copy link
Member

@cgonyeo cgonyeo commented Oct 22, 2015

An issue was found where remove commands will not emit an error if asked to remove a nonexistent item. This commit fixes that.

Additionally this commit adds test for the following commands:

  • annotation add
  • annotation remove
  • dependency add
  • dependency remove
  • end
  • environment add
  • environment remove
  • label add
  • label remove
  • mount add
  • mount remove
  • port add
  • port remove
  • set-exec
  • set-group
  • set-name
  • set-user

@cgonyeo
Copy link
Member Author

cgonyeo commented Oct 22, 2015

Addresses most of #24

@jonboulle
Copy link
Contributor

remove commands will not emit an error if asked to remove a nonexistent item.

SHOULD we emit an error in this case? There might be something to be said for commands being (optionally?) idempotent?

@cgonyeo
Copy link
Member Author

cgonyeo commented Oct 22, 2015

I think if you try to remove a mount point, and the mount point doesn't exist, it should at least say something. Perhaps just print a warning to stderr and have a 0 exit code?

@cgonyeo
Copy link
Member Author

cgonyeo commented Oct 22, 2015

Honestly I'd just prefer to leave it on the user to ignore the error if they're removing something they're not sure would exist.

@jonboulle
Copy link
Contributor

I am somewhat receptive to that but then it should be something consistent at the CLI layer (e.g. probably a particular error code) so that people can actually predictably ignore it.

/cc @krobertson thoughts?

TESTS_PATH="${REPO_PATH}/tests"

export ACBUILD_BIN="${PWD}/bin/acbuild"
export GOPATH=${PWD}/gopath
Copy link
Contributor

Choose a reason for hiding this comment

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

Quote, for consistency

@cgonyeo cgonyeo force-pushed the testing branch 3 times, most recently from 56081c0 to 2c8b05f Compare October 26, 2015 20:41
@cgonyeo
Copy link
Member Author

cgonyeo commented Oct 26, 2015

Should be ready for another pass. When acbuild is asked to remove something that doesn't exist the exit code will be 2, so that users can check what type of error was encountered. It'll also be easy going forward to add other error types with different exit codes.

var ErrNotFound = fmt.Errorf("element to be removed does not exist in this ACI")

// GetErrorCode will return the recommended exit code for acbuild given an error
func GetErrorCode(err error) int {
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting approach. I think this function should exist in the CLI package instead though (but still key off the lib.Err types)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I guess it does make more sense to be there. Fixed.

@cgonyeo
Copy link
Member Author

cgonyeo commented Oct 29, 2015

Should be ready for another pass.

@krobertson
Copy link
Contributor

LGTM. Oddly, github doesn't do syntax highlighting on the "set-user_test.go" files... maybe doesn't like the dash?

For the previous point about erroring when removing something not found, general command line behavior is to error. If you do just rm foo and foo doesn't exist, it'll be an error. Maybe a -f flag would make sense in the future?

This commit adds test for the following commands:
- annotation add
- annotation remove
- dependency add
- dependency remove
- end
- environment add
- environment remove
- label add
- label remove
- mount add
- mount remove
- port add
- port remove
- set-exec
- set-group
- set-name
- set-user
cgonyeo pushed a commit that referenced this pull request Oct 30, 2015
tests: added tests for all commands except run, write, copy, and version
@cgonyeo cgonyeo merged commit 9936c9d into containers:master Oct 30, 2015
@cgonyeo cgonyeo deleted the testing branch October 30, 2015 18:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants