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

Use voxpupuli-test #612

Merged
merged 4 commits into from
Apr 3, 2020
Merged

Use voxpupuli-test #612

merged 4 commits into from
Apr 3, 2020

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Dec 12, 2019

This gem has most of the duplication that used to be modulesynced. By moving it to a gem, the need for modulesyncing is greatly reduced.

Currently a draft since we should do some bikeshedding and the gem should live in the Voxpupuli org.

Ideally we'd end up with by default a single line in spec_helper.rb that is require 'voxpupuli/test/spec_helper'. Whenever a module then needs overrides, it can be done by unmanaging the file rather than the complex .sync.yml dance that's done now.

@@ -7,37 +7,6 @@ begin
rescue LoadError
end

PuppetLint.configuration.log_format = '%{path}:%{line}:%{check}:%{KIND}:%{message}'

desc 'Auto-correct puppet-lint offenses'
Copy link
Member Author

Choose a reason for hiding this comment

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

I've entirely dropped this since the puppetlabs_spec_helper task lint_fix should be sufficient.

Rake::Task[:lint_fix].invoke
end

desc 'Run acceptance tests'
Copy link
Member Author

Choose a reason for hiding this comment

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

We normally use the beaker job and I'd like to standardize on that.

t.pattern = 'spec/acceptance'
end

desc 'Run tests'
Copy link
Member Author

Choose a reason for hiding this comment

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

Also a customization that I've dropped for now.

desc 'Run tests'
task test: [:release_checks]

namespace :check do
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to the gem

Puppet::Util::Log.level = :debug
Puppet::Util::Log.newdestination(:console)
end
require 'voxpupuli/test/spec_helper'

if File.exist?(File.join(__dir__, 'default_module_facts.yml'))
Copy link
Member Author

Choose a reason for hiding this comment

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

I think that we can actually get rid of this since most have a small set. Once it's easy to unmanage the file, we should convert those to directly use add_custom_fact in those modules.

Copy link
Member

@bastelfreak bastelfreak left a comment

Choose a reason for hiding this comment

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

we should test this on a module, but it looks fine to me.

config_defaults.yml Outdated Show resolved Hide resolved
@bastelfreak bastelfreak force-pushed the vp-test-gem branch 2 times, most recently from d71d17c to 2d7e3e4 Compare February 9, 2020 17:24
@bastelfreak
Copy link
Member

@ekohl I:

  • release voxpupuli-test as 0.1.0
  • rebased your branch vp-test-gem
  • proposed a PR to voxpupuli-test to fix content we recently changed in modulesync_config

We we also add coveralls and simplecov as a runtime dependency to voxpupuli-test?

@bastelfreak bastelfreak force-pushed the vp-test-gem branch 2 times, most recently from 5b7e39c to 121e3df Compare February 9, 2020 18:25
@bastelfreak
Copy link
Member

@ekohl I tested this with the master branch of voxpupuli-test and it seems to work fine. Do you still consider this WIP or should we release voxpupuli-test and afterwards merge this PR?

@ekohl ekohl marked this pull request as ready for review February 10, 2020 16:41
@ekohl
Copy link
Member Author

ekohl commented Feb 10, 2020

IMHO voxpupuli/voxpupuli-test#4 should be merged and released. Also note I pushed another update after you did, but didn't test it so far. Other than that, I think this is ready for review.

@@ -141,7 +118,7 @@ spec/acceptance/nodesets/windows-2012-serverstandard-x64.yml:
spec/acceptance/nodesets/windows-2012R2-serverstandard-x64.yml:
delete: true
spec/spec_helper.rb:
mock_with: ':rspec'
mock_with: false
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we leave this out or keep it in as a reference on what to document?

Copy link
Member

Choose a reason for hiding this comment

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

I think more documentation is always a good approach

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit behind on this, why set mock_with to false?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because voxpupupli-test defaults to :rspec so there's no need to set it in the spec helper. This still allows an override but most modules shouldn't use it. This is in line with keeping the spec helper as minimal as possible. Ideally we'd standardize all our modules on rspec mocking and remove the need altogether, but we lack the resources to do so.

@alexjfisher
Copy link
Member

Is it worth creating a few test PRs across some of our modules?

@bastelfreak
Copy link
Member

@alexjfisher I'm currently releasing 0.2.0 of voxpupuli-test: voxpupuli/voxpupuli-test#6
Afterwards we can test this on some modules.

@ekohl
Copy link
Member Author

ekohl commented Feb 11, 2020

This week I won't have that much time to work on this, but a few test PRs are certainly a good idea. Especially modules that use lib and do coverage

@bastelfreak
Copy link
Member

I tested this at voxpupuli/puppet-ferm#94 and pushed one rubocop fix to this PR

@bastelfreak
Copy link
Member

I rebased this again and tested it on a few modules. I'm fine with merging it.

version: '~> 1.15.0'
- gem: mocha
version: '~> 1.4.0'
- gem: voxpupuli-test
Copy link
Member Author

Choose a reason for hiding this comment

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

What are people's thoughts on pinning this? Should we release a 1.0.0 and stick to semver?

Copy link
Member

Choose a reason for hiding this comment

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

since I didn't discover any further issues I'm 👍 for a 1.0.0 release + pinning to >= 1.0.0

ekohl added 2 commits April 3, 2020 12:08
This gem has most of the duplication that used to be modulesynced. By
moving it to a gem, the need for modulesyncing is greatly reduced.
@ekohl
Copy link
Member Author

ekohl commented Apr 3, 2020

Since I'm the author of the PR, GH won't allow me to approve it. However, I think it's ready to merge.

@bastelfreak bastelfreak merged commit 6521199 into voxpupuli:master Apr 3, 2020
@ekohl ekohl deleted the vp-test-gem branch April 3, 2020 10:18
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.

6 participants