Update acceptance tests, add EPEL test case for RedHat acceptance tests #308

wyardley · 2017-09-07T05:50:56Z

This does a few things:

Updates spec_helper_acceptance to use puppet_install_helper
Updates .travis.yml / .sync.yml to run acceptance tests in CI (CentOS 7 and Ubuntu 14)
Verifies some extra stuff, including the package source. This will fail as of now, because of the problem mentioned in EPEL now has dependency on http-parser #307 / https://bugzilla.redhat.com/show_bug.cgi?id=1481008

There are still some issues that running these tests exposes. It seems like Ubuntu 16.04 installs the Ubuntu version of nodejs vs. the nodesource one; I'm not trying to fix that one (or test for it) here.

bastelfreak · 2017-09-07T05:52:42Z

.travis.yml

+    env: PUPPET_INSTALL_TYPE=agent BEAKER_debug=true BEAKER_set=docker/centos-7 CHECK=beaker
+    services: docker
+    sudo: required
+  - rvm: 2.1.9


Can we use 2.4.1 here?

I think this would need to come from modulesync somehow?
https://github.com/voxpupuli/modulesync_config/blob/master/moduleroot/.travis.yml.erb#L39

Now that it's updated in modulesync, I'll give it a shot as an extra test.

bastelfreak · 2017-09-07T05:53:03Z

.sync.yml

@@ -1,3 +1,6 @@
 ---
 .travis.yml:
  secure: "SCQpiBB9qpZAjBRk+b9D3cSCQfYpDgHPOdOc7djfGeB5yn1UbGg7uW1zshrshb4QMLfUgvsL4LsT0CYj7ilBerghgeySF5JWuZdk05W/7Iudls4btbxdVjqtALR7p02mnk40qHTR1Tdb/j0gXW9uigU6nQU9iCP+Poa1KF6PXpk="
+  docker_sets:
+    - set: docker/ubuntu-14.04


Does 16.04 also work?

16.04 fails if we want the source to be nodesource, as it seems like it's supposed to be:
https://github.com/voxpupuli/puppet-nodejs/blob/master/manifests/params.pp#L86
16.04 gives me:

Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com>

I'm not sure if that's a bug (probably because Ubuntu 16 version is > nodesource one), or if it's intentional. If it's intentional, we could do some logic to look for different expected package sources depending on vendor.

Debian 82 did work.

ps - before the updates to spec_helper_acceptance, the tests were failing completely on multiple platforms, so this at least is an improvement, I think. Once we know what the expected behavior is, we can figure out how the tests should be (and if any fixes are needed in the actual module code).

could you add debian 8? Does 9 work as well

As an additional one, or instead of Ubuntu?
FWIW, I don't think modulesync provides Debian 9 yet, and I don't see it in the rake tasks
https://github.com/voxpupuli/modulesync_config/tree/master/moduleroot/spec/acceptance/nodesets

wyardley · 2017-09-07T06:01:10Z

ps - the failed test on CentOS 7 is expected (for now).

ekohl · 2017-09-07T10:00:44Z

spec/acceptance/class_spec.rb

+        Class['epel'] -> Class['nodejs']
+      }
+      EOS
+      # Run it twice and test for idempotency


I like https://github.com/theforeman/foreman-installer-modulesync/blob/0ee390953528a18b48ffdf94a97b71aa5029b1ad/moduleroot/spec/spec_helper_acceptance.rb.erb#L39-L47

I like this too, but can't use shared examples in an 'it' block, and maybe overkill to use within the same file? I am going to switch to the catch_failures / catch_changes pattern and simplify things a bit.

That does work: https://github.com/theforeman/puppet-foreman/blob/f0cfa1f39de13c1e937d23f2638db858eb7d2d27/spec/acceptance/foreman_cli_spec.rb#L33

ekohl · 2017-09-07T10:01:43Z

spec/acceptance/class_spec.rb

+      expect(apply_manifest(pp, catch_failures: true).exit_code).to be_zero
+    end
+
+    if fact('os.family') == 'RedHat' || fact('os.family') == 'Debian'


If you use ['RedHat', 'Debian'].include? fact('os.family') then you only need to get the fact once. Since it's retrieved from the VM/container over SSH this might be a good thing to consider.

ekohl · 2017-09-07T10:02:46Z

spec/acceptance/class_spec.rb

+  end
+
+  context 'repo_class => epel' do
+    if fact('os.family') == 'RedHat'


Can you squash this into the context: context 'repo_class => epel', if: fact('os.family') == 'RedHat' do (note that I'm not 100% sure this works so please test it)

wyardley · 2017-09-07T16:40:34Z

@ekohl @bastelfreak - How does that look now? I think I've addressed most of your (very helpful) feedback.
The tests now pass except for the expected failure.

ekohl · 2017-09-07T16:53:14Z

spec/acceptance/class_spec.rb

+        class { 'nodejs':
+          repo_class => '::epel',
+        }
+        class { 'epel': }


I think these 2 lines are redundant (or should be):

puppet-nodejs/manifests/init.pp

Lines 36 to 39 in 8fc47fb

include $repo_class

anchor { '::nodejs::begin': }

-> Class[$repo_class]

-> Class['::nodejs::install']

I wasn't sure about the history of that, so was hesitant to yank it otu; I will see if the test still works Ok with that removed, though, and commit if so.

We should also switch to 'contain' vs. anchor pattern (can do this in a separate PR)

We should also switch to 'contain' vs. anchor pattern (can do this in a separate PR)

+1

wyardley · 2017-09-07T22:18:27Z

In most recent push, I set the failing part as pending, we could then take the pending line out when CentOS 7.4 comes out?

wyardley · 2017-09-08T17:43:43Z

@ekohl: how's it look now?

bastelfreak · 2017-09-09T11:28:54Z

spec/spec_helper_acceptance.rb

@@ -31,7 +17,6 @@
    # Install module and dependencies
    hosts.each do |host|
      copy_module_to(host, source: proj_root, module_name: 'nodejs')
-      shell("/bin/touch #{default['puppetpath']}/hiera.yaml")
      on host, puppet('module install puppetlabs-apt --version 2.0.1'), acceptable_exit_codes: [0, 1]


should we remove the version foo here to get the latest?

I'd consider https://github.com/puppetlabs/beaker-module_install_helper since that also takes are of the copy_module_to line above if you're going to fix that.

Fine, I improved it and now you want more improvements? 😛 Ok, sold

ekohl · 2017-09-09T11:34:34Z

spec/acceptance/class_spec.rb

+  context 'default parameters' do
+    let(:pp) { "class { 'nodejs': }" }
+
+    it 'runs successfully' do


I would add the shared example for this but I can submit a PR for that later.

Cool, I didn't spend too long trying to get it to work (it wouldn't work inside an 'it' block).
I did see someone else create a self.foo in a test, and then use that where they wanted to reuse an item in the same test (vs. across different files).

The trick is that it already contains it blocks so you don't need that.

wyardley · 2017-09-09T20:37:17Z

Updated to use module_install_helper

…tests

… treydock-gpg_key module from acceptance tests

wyardley requested a review from bastelfreak September 7, 2017 05:50

bastelfreak reviewed Sep 7, 2017

View reviewed changes

wyardley mentioned this pull request Sep 7, 2017

EPEL now has dependency on http-parser #307

Closed

ekohl requested changes Sep 7, 2017

View reviewed changes

wyardley mentioned this pull request Sep 7, 2017

expected package source on Ubuntu 16 #309

Closed

ekohl reviewed Sep 7, 2017

View reviewed changes

wyardley mentioned this pull request Sep 8, 2017

Create some Beaker acceptance tests #139

Closed

bastelfreak reviewed Sep 9, 2017

View reviewed changes

ekohl reviewed Sep 9, 2017

View reviewed changes

ekohl approved these changes Sep 9, 2017

View reviewed changes

William Yardley added 2 commits September 9, 2017 13:49

Update acceptance tests and add EPEL test case for RedHat acceptance …

4ed75d0

…tests

Switch acceptance tests to use beaker-module_install_helper, and drop…

7612020

… treydock-gpg_key module from acceptance tests

ekohl approved these changes Sep 9, 2017

View reviewed changes

wyardley merged commit 56a6155 into voxpupuli:master Sep 9, 2017

wyardley deleted the update_acceptance_tests branch September 9, 2017 23:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update acceptance tests, add EPEL test case for RedHat acceptance tests #308

Update acceptance tests, add EPEL test case for RedHat acceptance tests #308

wyardley commented Sep 7, 2017

bastelfreak Sep 7, 2017

wyardley Sep 7, 2017

wyardley Sep 7, 2017

bastelfreak Sep 7, 2017

wyardley Sep 7, 2017

wyardley Sep 7, 2017

bastelfreak Sep 7, 2017

wyardley Sep 7, 2017

wyardley commented Sep 7, 2017

ekohl Sep 7, 2017

wyardley Sep 7, 2017

ekohl Sep 7, 2017

ekohl Sep 7, 2017

ekohl Sep 7, 2017

wyardley commented Sep 7, 2017

ekohl Sep 7, 2017

wyardley Sep 7, 2017

ekohl Sep 7, 2017

wyardley commented Sep 7, 2017

wyardley commented Sep 8, 2017

bastelfreak Sep 9, 2017

ekohl Sep 9, 2017

wyardley Sep 9, 2017

ekohl Sep 9, 2017

wyardley Sep 9, 2017

ekohl Sep 9, 2017

ekohl Sep 10, 2017

wyardley commented Sep 9, 2017

	include $repo_class
	anchor { '::nodejs::begin': }
	-> Class[$repo_class]
	-> Class['::nodejs::install']

Update acceptance tests, add EPEL test case for RedHat acceptance tests #308

Update acceptance tests, add EPEL test case for RedHat acceptance tests #308

Conversation

wyardley commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyardley commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyardley commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyardley commented Sep 7, 2017

wyardley commented Sep 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyardley commented Sep 9, 2017