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

the object 'options' provided by Commander doesn't have methods to te… #137

Merged
merged 5 commits into from
Jul 24, 2016

Conversation

sepulworld
Copy link
Owner

#134

I believe this is the route to take. @msabramo let me know what you think. The options object that Commander creates doesn't have methods to test responses using respond_to?

@@ -39,14 +39,12 @@ def password(_prompt)
options.port = 9000
options.username = 'me'
options.password = 'secret'
options.debug = true
Copy link
Owner Author

Choose a reason for hiding this comment

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

Need to revisit this. If we have this set the rake test output will include the debugging output (pretty noisy)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe default it to false?

@msabramo
Copy link
Contributor

msabramo commented Jul 23, 2016

IIRC, I added the respond_to? stuff because I had been trying to write tests and I was getting undefined method errors for Options#debug because it's not set by default.

Though maybe we can do this if by default we set

options.debug = false

in bin/aptly-cli.

config = AptlyCli::AptlyLoad.new.configure_with(nil)
cmd = AptlyCli::AptlyCommand.new(config, options)
cmd.config[:server].must_equal 'my-server'
cmd.config[:port].must_equal 9000
cmd.config[:username].must_equal 'me'
cmd.config[:password].must_equal 'secret'
cmd.config[:debug].must_equal true
cmd.config[:debug].must_equal nil

Choose a reason for hiding this comment

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

Trailing whitespace detected.

@coveralls
Copy link

coveralls commented Jul 23, 2016

Coverage Status

Coverage increased (+0.03%) to 98.907% when pulling bd8f35d on zmw/fix_options into 560a8ab on master.

@coveralls
Copy link

coveralls commented Jul 23, 2016

Coverage Status

Coverage decreased (-0.002%) to 98.876% when pulling 28f1db6 on zmw/fix_options into 560a8ab on master.

@coveralls
Copy link

coveralls commented Jul 23, 2016

Coverage Status

Coverage decreased (-0.002%) to 98.876% when pulling 28f1db6 on zmw/fix_options into 560a8ab on master.

@sepulworld
Copy link
Owner Author

Ok, I'm going to merge this since the options provided via command line are broken and this fixes it. Unless you have any objections? I still want to better understand the object respond_to issue as it seems it should have worked fine.

@msabramo
Copy link
Contributor

Yeah go for it since it fixes broken options. We can always reexamine later.

@msabramo
Copy link
Contributor

msabramo commented Jul 23, 2016

Weird that we didn't notice that the options were broken. I wonder if we should add higher level tests that invoke commands - those kind of tests should've detected that options weren't working I think.

In Python, I've used a module called scripttest on occasion to do this sort of testing. Not sure what folks do in Ruby for this sort of thing.

@sepulworld
Copy link
Owner Author

Yeah, I think higher level command checks makes good sense. At least to
cover global options.

On Sat, Jul 23, 2016, 3:12 PM Marc Abramowitz notifications@github.com
wrote:

Weird that we didn't notice that the options were broken. I wonder if we
should add higher level tests that invoke commands - those kind of tests
should've detected that options weren't working I think.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#137 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAg2O1w8sEN9JG9zqbfSxhp_FmkLWPH_ks5qYpG7gaJpZM4JTa-H
.

-Zane

@sepulworld sepulworld merged commit cfac194 into master Jul 24, 2016
@msabramo
Copy link
Contributor

Functional tests shouldn't be too difficult since you already have the Docker image for the aptly API server.

@sepulworld
Copy link
Owner Author

Right, I have used serverspec http://serverspec.org in the past and it probably will work well here.

@sepulworld sepulworld deleted the zmw/fix_options branch July 26, 2016 05:01
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.

4 participants