From a0878ccdb7476022a575fe4859e161af14786544 Mon Sep 17 00:00:00 2001 From: james-stocks Date: Mon, 24 Jul 2017 14:39:14 +0000 Subject: [PATCH 1/5] (SDK-313) Move audited acceptance tests to unit tests --- spec/acceptance/bundle_management_spec.rb | 17 ----------------- spec/acceptance/cli_spec.rb | 9 --------- spec/unit/pdk/cli_spec.rb | 11 +++++++++++ 3 files changed, 11 insertions(+), 26 deletions(-) delete mode 100644 spec/acceptance/cli_spec.rb diff --git a/spec/acceptance/bundle_management_spec.rb b/spec/acceptance/bundle_management_spec.rb index 91fbfbcf4..aa8e76b33 100644 --- a/spec/acceptance/bundle_management_spec.rb +++ b/spec/acceptance/bundle_management_spec.rb @@ -3,23 +3,6 @@ describe 'Managing Gemfile dependencies' do include_context 'in a new module', 'bundle_management' - context 'when there is no Gemfile.lock' do - before(:all) do - File.delete('Gemfile.lock') if File.exist?('Gemfile.lock') - # TODO: come up with a way to invoke only the bundler stuff without trying to run unit tests - # @result = shell_ex("#{path_to_pdk} ", chdir: target_dir) - end - - describe command('pdk test unit --debug') do - its(:exit_status) { is_expected.to eq 0 } - its(:stderr) { is_expected.to match(%r{Checking for missing Gemfile dependencies}i) } - - describe file('Gemfile.lock') do - it { is_expected.to be_file } - end - end - end - context 'when there is an invalid Gemfile' do before(:all) do FileUtils.mv('Gemfile', 'Gemfile.old', force: true) diff --git a/spec/acceptance/cli_spec.rb b/spec/acceptance/cli_spec.rb deleted file mode 100644 index fc113d49c..000000000 --- a/spec/acceptance/cli_spec.rb +++ /dev/null @@ -1,9 +0,0 @@ -require 'spec_helper_acceptance' - -describe 'Basic usage of the CLI' do - describe command('pdk --help') do - its(:exit_status) { is_expected.to eq 0 } - its(:stdout) { is_expected.to match(%r{NAME.*USAGE.*DESCRIPTION.*COMMANDS.*OPTIONS}m) } - its(:stderr) { is_expected.to match(%r{\A\Z}) } - end -end diff --git a/spec/unit/pdk/cli_spec.rb b/spec/unit/pdk/cli_spec.rb index b267463cf..027ce5bce 100644 --- a/spec/unit/pdk/cli_spec.rb +++ b/spec/unit/pdk/cli_spec.rb @@ -1,6 +1,17 @@ require 'spec_helper' describe PDK::CLI do + context 'when invoking help' do + it 'outputs basic help' do + expect($stdout).to receive(:puts).with(a_string_matching(%r{NAME.*USAGE.*DESCRIPTION.*COMMANDS.*OPTIONS}m)) + expect { + described_class.run(['--help']) + }.to raise_error(SystemExit) { |error| + expect(error.status).to eq(0) + } + end + end + context 'when provided an invalid report format' do it 'informs the user and exits' do expect(logger).to receive(:fatal).with(a_string_matching(%r{'non_existant_format'.*valid report format})) From 5c25d812eca0ad2d02911dd639c53b15829bdb75 Mon Sep 17 00:00:00 2001 From: james-stocks Date: Tue, 25 Jul 2017 13:28:39 +0000 Subject: [PATCH 2/5] (SDK-313) Replace acceptance/test_spec.rb with a unit test --- spec/acceptance/test_spec.rb | 21 --------------------- spec/acceptance/validate_spec.rb | 4 ---- spec/unit/pdk/cli_spec.rb | 13 +++++++++++++ 3 files changed, 13 insertions(+), 25 deletions(-) delete mode 100644 spec/acceptance/test_spec.rb delete mode 100644 spec/acceptance/validate_spec.rb diff --git a/spec/acceptance/test_spec.rb b/spec/acceptance/test_spec.rb deleted file mode 100644 index ae85b1300..000000000 --- a/spec/acceptance/test_spec.rb +++ /dev/null @@ -1,21 +0,0 @@ -require 'spec_helper_acceptance' -require 'fileutils' - -describe 'Using the test command' do - context 'not within a module directory' do - before(:all) do - Dir.mkdir('not_a_module') || raise - Dir.chdir('not_a_module') - end - - after(:all) do - Dir.chdir('..') - FileUtils.rm_rf('not_a_module') - end - - describe command('pdk test unit') do - its(:exit_status) { is_expected.not_to eq(0) } - its(:stderr) { is_expected.to match(%r{no metadata\.json found}i) } - end - end -end diff --git a/spec/acceptance/validate_spec.rb b/spec/acceptance/validate_spec.rb deleted file mode 100644 index 57d86ca18..000000000 --- a/spec/acceptance/validate_spec.rb +++ /dev/null @@ -1,4 +0,0 @@ -require 'spec_helper_acceptance' - -describe 'pdk validate', module_command: true do # rubocop:disable RSpec/EmptyExampleGroup -end diff --git a/spec/unit/pdk/cli_spec.rb b/spec/unit/pdk/cli_spec.rb index 027ce5bce..b7d482435 100644 --- a/spec/unit/pdk/cli_spec.rb +++ b/spec/unit/pdk/cli_spec.rb @@ -12,6 +12,19 @@ end end + ['validate', 'test unit', 'bundle'].each do |command| + context "when #{command} command used but not in a module folder" do + it 'informs the user that this is not a module folder' do + expect { + described_class.run(command.split(' ')) + }.to raise_error(SystemExit) { |error| + expect(error.status).not_to eq(0) + expect(error.cause.to_s).to match(%r{no metadata\.json found}i) + } + end + end + end + context 'when provided an invalid report format' do it 'informs the user and exits' do expect(logger).to receive(:fatal).with(a_string_matching(%r{'non_existant_format'.*valid report format})) From e78e865af6b4e4652af838bd46328c5a9ab84d2b Mon Sep 17 00:00:00 2001 From: james-stocks Date: Tue, 25 Jul 2017 13:35:50 +0000 Subject: [PATCH 3/5] (SDK-313) Refine validation acceptance tests Remove tests that are either: * Sufficiently covered at the unit test level, or * Are 'happy path' tests that are redundant on the negative test cases that accompany them --- spec/acceptance/validate_all_spec.rb | 5 --- spec/acceptance/validate_metadata_spec.rb | 38 ++----------------- spec/acceptance/validate_ruby_spec.rb | 45 ----------------------- 3 files changed, 4 insertions(+), 84 deletions(-) diff --git a/spec/acceptance/validate_all_spec.rb b/spec/acceptance/validate_all_spec.rb index ee6c1c330..e191617d6 100644 --- a/spec/acceptance/validate_all_spec.rb +++ b/spec/acceptance/validate_all_spec.rb @@ -17,11 +17,6 @@ class validate_all { } end end - describe command('pdk validate --list') do - its(:exit_status) { is_expected.to eq(0) } - its(:stderr) { is_expected.to match(%r{Available validators: metadata, puppet, ruby}i) } - end - describe command('pdk validate') do its(:exit_status) { is_expected.to eq(0) } its(:stderr) { is_expected.to match(%r{Running all available validators}i) } diff --git a/spec/acceptance/validate_metadata_spec.rb b/spec/acceptance/validate_metadata_spec.rb index 9ff1d0c22..827cc5491 100644 --- a/spec/acceptance/validate_metadata_spec.rb +++ b/spec/acceptance/validate_metadata_spec.rb @@ -3,36 +3,6 @@ describe 'Running metadata validation' do let(:spinner_text) { %r{checking metadata}i } - context 'with a fresh module' do - include_context 'in a new module', 'metadata_validation_module' - - describe command('pdk validate metadata') do - its(:exit_status) { is_expected.to eq(0) } - its(:stdout) { is_expected.to match(%r{\A\Z}) } - its(:stderr) { is_expected.to match(spinner_text) } - end - - describe command('pdk validate metadata --format junit') do - its(:exit_status) { is_expected.to eq(0) } - its(:stderr) { is_expected.to match(spinner_text) } - it_behaves_like :it_generates_valid_junit_xml - - its(:stdout) do - is_expected.to have_junit_testsuite('metadata-json-lint').with_attributes( - 'failures' => eq(0), - 'tests' => eq(1), - ) - end - - its(:stdout) do - is_expected.to have_junit_testcase.in_testsuite('metadata-json-lint').with_attributes( - 'classname' => 'metadata-json-lint', - 'name' => 'metadata.json', - ).that_passed - end - end - end - context 'with a metadata violation' do include_context 'in a new module', 'metadata_violation_module' @@ -123,17 +93,17 @@ its(:stderr) { is_expected.to match(spinner_text) } its(:stdout) do - is_expected.to have_xpath('/testsuites/testsuite[@name="metadata-json-lint"]/testcase').with_attributes( + is_expected.to have_junit_testcase.in_testsuite('metadata-json-lint').with_attributes( 'classname' => 'metadata-json-lint', 'name' => 'metadata.json', - ) + ).that_passed end its(:stdout) do - is_expected.to have_xpath('/testsuites/testsuite[@name="metadata-json-lint"]/testcase').with_attributes( + is_expected.to have_junit_testcase.in_testsuite('metadata-json-lint').with_attributes( 'classname' => 'metadata-json-lint.dependencies', 'name' => 'broken.json', - ) + ).that_failed end end end diff --git a/spec/acceptance/validate_ruby_spec.rb b/spec/acceptance/validate_ruby_spec.rb index 21e609df3..886b9d604 100644 --- a/spec/acceptance/validate_ruby_spec.rb +++ b/spec/acceptance/validate_ruby_spec.rb @@ -3,51 +3,6 @@ describe 'pdk validate ruby', module_command: true do let(:junit_xsd) { File.join(RSpec.configuration.fixtures_path, 'JUnit.xsd') } - context 'with a fresh module' do - include_context 'in a new module', 'validate_ruby_module' - - example_rb = File.join('spec', 'example.rb') - - before(:all) do - File.open(example_rb, 'w') do |f| - f.puts "require 'filepath'" - end - end - - describe command('pdk validate ruby') do - its(:exit_status) { is_expected.to eq(0) } - its(:stdout) { is_expected.to match(%r{\A\Z}) } - its(:stderr) { is_expected.to match(%r{Checking Ruby code style}i) } - end - - describe command('pdk validate ruby --format junit') do - its(:exit_status) { is_expected.to eq(0) } - its(:stderr) { is_expected.to match(%r{checking ruby code style}i) } - it_behaves_like :it_generates_valid_junit_xml - - its(:stdout) do - is_expected.to have_junit_testsuite('rubocop').with_attributes( - 'failures' => eq(0), - 'tests' => a_value > 0, - ) - end - - its(:stdout) do - is_expected.to have_junit_testcase.in_testsuite('rubocop').with_attributes( - 'classname' => 'rubocop', - 'name' => example_rb, - ).that_passed - end - - its(:stdout) do - is_expected.to have_junit_testcase.in_testsuite('rubocop').with_attributes( - 'classname' => 'rubocop', - 'name' => File.join('spec', 'spec_helper.rb'), - ).that_passed - end - end - end - context 'with a style violation' do include_context 'in a new module', 'foo' From 269321c8f4d2acc3a25b06b3522c2ec89fe7f6d5 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 27 Jul 2017 15:01:47 +0100 Subject: [PATCH 4/5] (maint) Increase unit coverage for test and test unit --- spec/unit/cli/test/unit_spec.rb | 98 +++++++++++++++++++++++++++++++++ spec/unit/cli/test_spec.rb | 15 +++++ 2 files changed, 113 insertions(+) create mode 100644 spec/unit/cli/test/unit_spec.rb create mode 100644 spec/unit/cli/test_spec.rb diff --git a/spec/unit/cli/test/unit_spec.rb b/spec/unit/cli/test/unit_spec.rb new file mode 100644 index 000000000..dc193aeb5 --- /dev/null +++ b/spec/unit/cli/test/unit_spec.rb @@ -0,0 +1,98 @@ +require 'spec_helper' +require 'pdk/tests/unit' + +describe 'Running `pdk test unit`' do + subject(:test_unit_cmd) { PDK::CLI.instance_variable_get(:@test_unit_cmd) } + + it { is_expected.not_to be_nil } + + context 'with --help' do + it do + begin + expect { + PDK::CLI.run(['test', 'unit', '--help']) + }.to output(%r{^USAGE\s+pdk test unit}m).to_stdout + rescue SystemExit => e + expect(e.status).to eq 0 + end + end + end + + context 'when listing tests' do + let(:args) { ['--list'] } + + before(:each) do + expect(PDK::CLI::Util).to receive(:ensure_in_module!).with(no_args).once + end + + context 'when no tests are found' do + before(:each) do + expect(PDK::Test::Unit).to receive(:list).with(no_args).once.and_return([]) + expect($stdout).to receive(:puts).with(%r{No examples found}) + end + + it { test_unit_cmd.run_this(args) } + end + + context 'when some tests are found' do + let(:test_list) { [{ id: 'first_id', full_description: 'first_description' }, { id: 'second_id', full_description: 'second_description' }] } + + before(:each) do + expect(PDK::Test::Unit).to receive(:list).with(no_args).once.and_return(test_list) + expect($stdout).to receive(:puts).with('Examples:') + expect($stdout).to receive(:puts).with(%r{first_id\tfirst_description}) + expect($stdout).to receive(:puts).with(%r{second_id\tsecond_description}) + end + + it { test_unit_cmd.run_this(args) } + end + end + + context 'when running tests' do + context 'when tests pass' do + before(:each) do + expect(PDK::CLI::Util).to receive(:ensure_in_module!).with(no_args).once + expect(PDK::Test::Unit).to receive(:invoke).with(instance_of(PDK::Report), hash_including(:tests)).once.and_return(0) + end + + it 'returns 0' do + begin + test_unit_cmd.run_this([]) + rescue SystemExit => e + expect(e.status).to eq 0 + end + end + end + + context 'when tests pass, with a format option' do + before(:each) do + expect(PDK::CLI::Util).to receive(:ensure_in_module!).with(no_args).once + expect(PDK::CLI::Util::OptionNormalizer).to receive(:report_formats).with(['text:results.txt']).and_return([{ method: :write_text, target: 'results.txt' }]).twice + expect(PDK::Test::Unit).to receive(:invoke).with(instance_of(PDK::Report), hash_including(:tests)).once.and_return(0) + end + + it 'returns 0' do + begin + test_unit_cmd.run_this(['--format=text:results.txt']) + rescue SystemExit => e + expect(e.status).to eq 0 + end + end + end + + context 'when tests fail' do + before(:each) do + expect(PDK::CLI::Util).to receive(:ensure_in_module!).with(no_args).once + expect(PDK::Test::Unit).to receive(:invoke).with(instance_of(PDK::Report), hash_including(:tests)).once.and_return(1) + end + + it 'does not return 0' do + begin + test_unit_cmd.run_this([]) + rescue SystemExit => e + expect(e.status).not_to eq 0 + end + end + end + end +end diff --git a/spec/unit/cli/test_spec.rb b/spec/unit/cli/test_spec.rb new file mode 100644 index 000000000..5fcad7a80 --- /dev/null +++ b/spec/unit/cli/test_spec.rb @@ -0,0 +1,15 @@ +require 'spec_helper' + +describe 'Running `pdk test`' do + subject { PDK::CLI.instance_variable_get(:@test_cmd) } + + it { is_expected.not_to be_nil } + + context 'when no arguments or options are provided' do + it do + expect { + PDK::CLI.run(['test']) + }.to output(%r{^USAGE\s+pdk test}m).to_stdout + end + end +end From bb2c17071f9175026b9e5f820d9e431390195791 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Tue, 8 Aug 2017 15:18:06 +0100 Subject: [PATCH 5/5] (SDK-313) refactor cli/tests/unit_spec.rb * extract common pre-conditions * change SystemExit to detect missing exceptions * move explicit $stdout expectations to output matchers --- spec/unit/cli/test/unit_spec.rb | 115 +++++++++++++++----------------- 1 file changed, 53 insertions(+), 62 deletions(-) diff --git a/spec/unit/cli/test/unit_spec.rb b/spec/unit/cli/test/unit_spec.rb index dc193aeb5..7d60084fe 100644 --- a/spec/unit/cli/test/unit_spec.rb +++ b/spec/unit/cli/test/unit_spec.rb @@ -1,96 +1,87 @@ require 'spec_helper' require 'pdk/tests/unit' -describe 'Running `pdk test unit`' do +describe '`pdk test unit`' do subject(:test_unit_cmd) { PDK::CLI.instance_variable_get(:@test_unit_cmd) } it { is_expected.not_to be_nil } context 'with --help' do it do - begin - expect { - PDK::CLI.run(['test', 'unit', '--help']) - }.to output(%r{^USAGE\s+pdk test unit}m).to_stdout - rescue SystemExit => e + expect { + PDK::CLI.run(['test', 'unit', '--help']) + }.to raise_error(SystemExit) { |e| expect(e.status).to eq 0 - end + }.and output(%r{^USAGE\s+pdk test unit}m).to_stdout end end - context 'when listing tests' do - let(:args) { ['--list'] } - + context 'when executing' do before(:each) do expect(PDK::CLI::Util).to receive(:ensure_in_module!).with(no_args).once end - context 'when no tests are found' do - before(:each) do - expect(PDK::Test::Unit).to receive(:list).with(no_args).once.and_return([]) - expect($stdout).to receive(:puts).with(%r{No examples found}) - end - - it { test_unit_cmd.run_this(args) } - end + context 'when listing tests' do + let(:args) { ['--list'] } - context 'when some tests are found' do - let(:test_list) { [{ id: 'first_id', full_description: 'first_description' }, { id: 'second_id', full_description: 'second_description' }] } + context 'when no tests are found' do + before(:each) do + expect(PDK::Test::Unit).to receive(:list).with(no_args).once.and_return([]) + end - before(:each) do - expect(PDK::Test::Unit).to receive(:list).with(no_args).once.and_return(test_list) - expect($stdout).to receive(:puts).with('Examples:') - expect($stdout).to receive(:puts).with(%r{first_id\tfirst_description}) - expect($stdout).to receive(:puts).with(%r{second_id\tsecond_description}) + it { expect { test_unit_cmd.run_this(args) }.to output(%r{No examples found}m).to_stdout } end - it { test_unit_cmd.run_this(args) } - end - end - - context 'when running tests' do - context 'when tests pass' do - before(:each) do - expect(PDK::CLI::Util).to receive(:ensure_in_module!).with(no_args).once - expect(PDK::Test::Unit).to receive(:invoke).with(instance_of(PDK::Report), hash_including(:tests)).once.and_return(0) - end + context 'when some tests are found' do + let(:test_list) { [{ id: 'first_id', full_description: 'first_description' }, { id: 'second_id', full_description: 'second_description' }] } - it 'returns 0' do - begin - test_unit_cmd.run_this([]) - rescue SystemExit => e - expect(e.status).to eq 0 + before(:each) do + expect(PDK::Test::Unit).to receive(:list).with(no_args).once.and_return(test_list) end + + it { expect { test_unit_cmd.run_this(args) }.to output(%r{Examples:\nfirst_id\tfirst_description\nsecond_id\tsecond_description}m).to_stdout } end end - context 'when tests pass, with a format option' do - before(:each) do - expect(PDK::CLI::Util).to receive(:ensure_in_module!).with(no_args).once - expect(PDK::CLI::Util::OptionNormalizer).to receive(:report_formats).with(['text:results.txt']).and_return([{ method: :write_text, target: 'results.txt' }]).twice - expect(PDK::Test::Unit).to receive(:invoke).with(instance_of(PDK::Report), hash_including(:tests)).once.and_return(0) - end + context 'when running tests' do + context 'when tests pass' do + before(:each) do + expect(PDK::Test::Unit).to receive(:invoke).with(instance_of(PDK::Report), hash_including(:tests)).once.and_return(0) + end - it 'returns 0' do - begin - test_unit_cmd.run_this(['--format=text:results.txt']) - rescue SystemExit => e - expect(e.status).to eq 0 + it do + expect { + test_unit_cmd.run_this([]) + }.to raise_error(SystemExit) { |e| + expect(e.status).to eq 0 + } end - end - end - context 'when tests fail' do - before(:each) do - expect(PDK::CLI::Util).to receive(:ensure_in_module!).with(no_args).once - expect(PDK::Test::Unit).to receive(:invoke).with(instance_of(PDK::Report), hash_including(:tests)).once.and_return(1) + context 'with a format option' do + before(:each) do + expect(PDK::CLI::Util::OptionNormalizer).to receive(:report_formats).with(['text:results.txt']).and_return([{ method: :write_text, target: 'results.txt' }]).twice + end + it do + expect { + test_unit_cmd.run_this(['--format=text:results.txt']) + }.to raise_error(SystemExit) { |e| + expect(e.status).to eq 0 + } + end + end end - it 'does not return 0' do - begin - test_unit_cmd.run_this([]) - rescue SystemExit => e - expect(e.status).not_to eq 0 + context 'when tests fail' do + before(:each) do + expect(PDK::Test::Unit).to receive(:invoke).with(instance_of(PDK::Report), hash_including(:tests)).once.and_return(1) + end + + it do + expect { + test_unit_cmd.run_this([]) + }.to raise_error(SystemExit) { |e| + expect(e.status).not_to eq 0 + } end end end