Skip to content
This repository was archived by the owner on Oct 24, 2022. It is now read-only.

fixes #15916 - infrastructure to switch tests to use vcr #116

Merged
thomasmckay merged 1 commit intoKatello:masterfrom
thomasmckay:15916-vcr
Aug 2, 2016
Merged

fixes #15916 - infrastructure to switch tests to use vcr #116
thomasmckay merged 1 commit intoKatello:masterfrom
thomasmckay:15916-vcr

Conversation

@thomasmckay
Copy link
Member

No description provided.

@thomasmckay thomasmckay changed the title fixes #15916 - wip [WIP] fixes #15916 - wip Jul 29, 2016
@thomasmckay
Copy link
Member Author

Ran w/

rake test mode=all record=false test=resources/settings

@thomasmckay
Copy link
Member Author

$ rake test logging=true mode=all record=true test=resources/settings
Debugging not enabled.
Running tests for: resources/settings
Could not load the API description from the server
  - is the server down?
  - was 'foreman-rake apipie:cache' run on the server when using apipie cache? (typical production settings)
rake aborted!
Could not load data from http://katello:3000
 - is your server down?
 - was rake apipie:cache run when using apipie cache? (typical production settings)
/home/thomasmckay/code/hammer-cli-foreman/lib/hammer_cli_foreman.rb:144:in `rescue in <module:HammerCLIForeman>'
/home/thomasmckay/code/hammer-cli-foreman/lib/hammer_cli_foreman.rb:25:in `<module:HammerCLIForeman>'
/home/thomasmckay/code/hammer-cli-foreman/lib/hammer_cli_foreman.rb:5:in `<top (required)>'
/home/thomasmckay/code/hammer-cli-csv/lib/hammer_cli_csv.rb:5:in `require'
/home/thomasmckay/code/hammer-cli-csv/lib/hammer_cli_csv.rb:5:in `<module:HammerCLICsv>'
/home/thomasmckay/code/hammer-cli-csv/lib/hammer_cli_csv.rb:4:in `<top (required)>'
/home/thomasmckay/code/hammer-cli-csv/test/csv_test_helper.rb:26:in `require'
/home/thomasmckay/code/hammer-cli-csv/test/csv_test_helper.rb:26:in `<top (required)>'
/home/thomasmckay/code/hammer-cli-csv/test/resources/settings_test.rb:4:in `require'
/home/thomasmckay/code/hammer-cli-csv/test/resources/settings_test.rb:4:in `<top (required)>'
/home/thomasmckay/code/hammer-cli-csv/test/test_runner.rb:163:in `require'
/home/thomasmckay/code/hammer-cli-csv/test/test_runner.rb:163:in `run_tests'
/home/thomasmckay/code/hammer-cli-csv/Rakefile:43:in `block (3 levels) in <top (required)>'
/home/thomasmckay/code/hammer-cli-csv/Rakefile:77:in `block in <top (required)>'
/home/thomasmckay/.rvm/gems/ruby-2.0.0-p481@hammer/bin/ruby_executable_hooks:15:in `eval'
/home/thomasmckay/.rvm/gems/ruby-2.0.0-p481@hammer/bin/ruby_executable_hooks:15:in `<main>'
Tasks: TOP => test:resources
(See full trace by running task with --trace)
Coverage report generated for MiniTest to /home/thomasmckay/code/coverage. 1319 / 3234 LOC (40.79%) covered.
Run options: --seed 11559

# Running tests:

Running Suite MiniTest::Spec - :test 
Completed Running Suite MiniTest::Spec - :test 


Finished tests in 0.000318s, 0.0000 tests/s, 0.0000 assertions/s.

0 tests, 0 assertions, 0 failures, 0 errors, 0 skips

@akofink
Copy link
Member

akofink commented Jul 29, 2016

Was there an rfc for this? I totally support adding tests, but let's discuss the plan. hammer-cli-katello doesn't use VCR, simply the apipie cache as its input. Could that be an option? Are there already bats tests? I can't seem to find any in https://github.com/Katello/forklift/tree/master/bats. Essentially, I'd like to see some discussion on why we think VCR is the way forward and also how this test suite can be maintainable with an app API that could potentially change.

@thomasmckay
Copy link
Member Author

I'd like to see bats tests as well, yes. This would catch the myriad issues I've had w/ the API changing.

For VCR, I'd like tests that confirm that export/import remains consistent, especially since there is going to be some logic added around attaching subscriptions to hosts. Running in live mode is equivalent to the tests I have now.

What specifically do you see as potential issues in adding VCR? I'm happy to adjust the strategy. In the end, I want live tests as well as non-live.

@akofink
Copy link
Member

akofink commented Jul 30, 2016

I'm not really familiar with VCR nor have any qualms about it; just wondering why we've chosen it. Do we already use it in some other repository?

Doing a bit of research on web mocking frameworks, there are many to choose from. It seems like webmock is lighter and more maintainable, according to this blog post from 2015, but it requires a bit more work up front (Yes, it's only one opinion/experience).

I do find it strange that a test framework would have such mediocre coverage. I'm playing a bit of devil's advocate if you can't tell. 😈 As we discussed via IRC last week, we should totally pick the easiest way to get tests in hammer-cli-csv first so we get the most bang for the buck, so to speak, and ideally add more fine-grained or maintainable tests over time. If VCR will meet that requirement, so be it. 👍

@thomasmckay
Copy link
Member Author

@akofink - Definitely bring discussion to my thread on foreman-dev, if desired. VCR is used in our repos; the example I was pointed to as a good example is https://github.com/Katello/runcible

@thomasmckay thomasmckay changed the title [WIP] fixes #15916 - wip [WIP] fixes #15916 - infrastructure to switch tests to use vcr Aug 2, 2016
@daviddavis
Copy link

daviddavis commented Aug 2, 2016

@akofink I'm not sure I would call VCR a web mocking framework. It's more of a tool that allows you to record actual web requests and then we use them as mocks for webmock. We use VCR quite extensively in Katello's repos (katello, runcible, etc) and I haven't heard of any problems. Our requests are complex so being able to record actual requests and use that as mocking data is much easier than manually constructing them in the actual test code.

@thomasmckay thomasmckay changed the title [WIP] fixes #15916 - infrastructure to switch tests to use vcr fixes #15916 - infrastructure to switch tests to use vcr Aug 2, 2016
@thomasmckay
Copy link
Member Author

[test]

@thomasmckay
Copy link
Member Author

@akofink @daviddavis all set for ack/nack

@thomasmckay
Copy link
Member Author

[test]

README.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This badge is wrong. It should be [![Build Status](https://travis-ci.org/Katello/hammer-cli-csv.svg?branch=master)](https://travis-ci.org/Katello/hammer-cli-csv)

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see. I'll change.

Copy link
Member

Choose a reason for hiding this comment

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

Like this:

Build Status

@akofink
Copy link
Member

akofink commented Aug 2, 2016

I believe you need to add a default rake task. It should probably run rubocop, then the tests.

@thomasmckay thomasmckay force-pushed the 15916-vcr branch 2 times, most recently from 4aee9fb to a9d0cc5 Compare August 2, 2016 20:29
@akofink
Copy link
Member

akofink commented Aug 2, 2016

APJ thanks @thomasmckay!

Switching to VCR will allow both running against a live server (as the existing tests do now) or to run in pre-recorded mode.

Setting initial code coverage to 26%

Added default rake task to run rubocop and test
@thomasmckay thomasmckay merged commit 32d09ad into Katello:master Aug 2, 2016
@thomasmckay thomasmckay deleted the 15916-vcr branch August 2, 2016 20:37
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.

3 participants

Comments