Refactor and add tests #6

alex-harvey-z3q · 2016-03-31T13:54:17Z

PR uses #5 by @petems as a starting point.

Fixes tests.
Newer boilerplate.
rancher::node fails compilation if empty input provided.
Updated nodeset for latest PL CentOS 7 box (CentOS 7.2).
Removes Gemfile.lock from git.
Modified .travis.yml.

alex-harvey-z3q · 2016-03-31T13:57:53Z

manifests/management.pp


-class rancher::management {
+  require ::docker


I am not sure what the thinking is here? Can't we just include?

Using require explictly makes sure that the docker class has been applied before all the resources in the class are. Using include does not, details here: https://docs.puppetlabs.com/puppet/latest/reference/lang_classes.html#using-require

Thats super cool, did not know about that, thanks @petems !

I meant to ask about the explicit top scope i.e. why require ::docker and not require docker? I see people doing this and I haven't been able to find out why. I'll go through and do it consistently if we agree this is preferred.

ozbillwang · 2016-03-31T23:27:09Z

Thanks, @alexharv074 That's a real big improvement.

petems · 2016-03-31T23:40:33Z

.travis.yml

+matrix:
+  include:
+  - rvm: 1.9.3
+    env: PUPPET_GEM_VERSION="~> 3.2.0"


Is there a reason you're testing 3.2? It's a pretty old, there's not much benefit testing it because it's missing out a lot of essentials such as contain

Generally we test 3.8, 3.8 Future parser, latest 4.0?

Should speed up the builds as well

Sure thing, I'll remove that.

petems · 2016-03-31T23:41:28Z

@alexharv074 Looks good! Got some questions about some of your changes, but I agree with most of it! 😄

alex-harvey-z3q · 2016-04-01T01:42:34Z

Ok, I just had a look into the puppet install helper gem. It's controlled by environment variables by the looks of it. I'll refactor to use the puppet install helper. Glad someone's finally cleared this up for me. :)

alex-harvey-z3q · 2016-04-01T04:02:24Z

@petems I have rebased including most of your suggestions.

I restored your version of the spec_helper_acceptance.rb.
Is there any agreed upon version of .travis.yml ? All the Puppet Labs modules seem to have something different in there. What I have in there now seems to be OK otherwise.
This serverspec test is failing:

    describe docker_container('rancher/server') do
      it { is_expected.to be_running }
    end

I believe there's a bug in serverspec and I'm still investigating.

I'm awaiting feedback re: expected output from curl 0.0.0.0:8080. I'm getting this and I'd like someone who knows more about Rancher to advise:

{
  "type": "collection",
  "resourceType": "apiVersion",
  "links": {
    "self": "http://0.0.0.0:8080/",
    "latest": "http://0.0.0.0:8080/v1"
  },
  "createTypes": {},
  "actions": {},
  "data": [
    {
      "id": "v1",
      "type": "apiVersion",
      "links": {
        "self": "http://0.0.0.0:8080/v1"
      },
      "actions": {}
    }
  ],
  "sortLinks": {},
  "pagination": null,
  "sort": null,
  "filters": {},
  "createDefaults": {}
}

Still to do, I see the build is still broken and I'll investigate why. Probably because I'm testing against old Puppets.

alex-harvey-z3q · 2016-04-01T08:20:47Z

Ok, to the Serverspec bug.

I have confirmed that a new feature is to be released in Docker 1.11 release notes:

docker inspect now also returns a new State field containing the container state in a human readable way (i.e. one of created, restarting, running, paused, exited or dead)(#18966)

The version of Docker that has installed is 1.10.3 and it doesn't have this feature.

Meanwhile Serverspec has a method running? here that assumes that the State field already exists.

The really weird part is that this was added in Serverspec on Oct 12, 2014.

I am tempted to simply not do this test, or use a command instead.

Any feedback on this would be welcome, as this doesn't make any sense to me. I feel I must have misread or misunderstood something but can't see where.

alex-harvey-z3q · 2016-04-01T09:04:19Z

Ok, I have removed the troublesome test, which is really redundant anyway considering the test that follows it.

alex-harvey-z3q · 2016-04-01T09:04:55Z

@petems @nickschuch AFAICT, the only outstanding issue is Pete's expectation that curl 0.0.0.0:8080 should contain /\<title>rancher<\/title>/ whereas I'm really seeing what I posted above.

petems · 2016-04-01T09:47:19Z

spec/acceptance/management_spec.rb

+
+    context 'is expected to be running on the default port' do
+      before :each do
+        sleep 5 # Give it time to start.


Unfortunately, due to the nature of how beaker-rspec runs, this won't delay the check. This sleep will just happen when the Ruby spec runs:

centos-72-x64 10:38:25$ /bin/sh -c curl\ 0.0.0.0:8080 % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 curl: (56) Recv failure: Connection reset by peer

You have to put a sleep exec into the host itself to work:

describe command('sleep 5 && echo "Give Rancher time to start"') do its(:exit_status) { should eq 0 } end

How about something along these lines:

describe Windows_scheduled_task('puppet upgrade task') do it { should exist } end #This will fail if your laptop (and therefor the Vagrant vm) is not running on AC power describe package('puppet') do it { sleep(240) #Wait for the task to execute should be_installed.with_version('3.5.1') } end

From Liam Bennett's blog here.

I believe this still won't work. From my test just now, even with a super long wait, it doesn't work 😢

alex-harvey-z3q · 2016-04-01T09:53:12Z

Oh, I should add that Rancher seems to be working fine when I point a browser at port 8080.

petems · 2016-04-01T09:56:59Z

Yeah it is working, when I test it, it's running. But depending on the speed of your VM it might not be ready at the time of the spec running.

For example, see my history of it not running during the test, but OK a few seconds after the acceptance test:

Failures:

  1) rancher::management puppet apply is expected to be running on the default port Command "curl 0.0.0.0:8080" stdout should match /self.*0.0.0.0:8080/
     Failure/Error: its(:stdout) { is_expected.to match /self.*0.0.0.0:8080/ }
       expected "" to match /self.*0.0.0.0:8080/
       Diff:
       @@ -1,2 +1,2 @@
       -/self.*0.0.0.0:8080/
       +""


     # ./spec/acceptance/management_spec.rb:22:in `block (5 levels) in <top (required)>'

Finished in 2 minutes 55.2 seconds (files took 1 minute 35.4 seconds to load)
2 examples, 1 failure

Failed examples:

rspec ./spec/acceptance/management_spec.rb:22 # rancher::management puppet apply is expected to be running on the default port Command "curl 0.0.0.0:8080" stdout should match /self.*0.0.0.0:8080/

10:38:25  ! ~/projects/puppet-rancher (refactor_2 *$) $ cd .vagrant/beaker_vagrant_files/default.yml/
10:42:45 ~/projects/puppet-rancher/.vagrant/beaker_vagrant_files/default.yml (refactor_2 *$) $ vagrant ssh centos-72-x64
Last login: Fri Apr  1 09:35:15 2016 from 10.0.2.2
[vagrant@centos-72-x64 ~]$ sudo -s
[root@centos-72-x64 vagrant]# curl 0.0.0.0:8080
{"type":"collection","resourceType":"apiVersion","links":{"self":"http://0.0.0.0:8080/","latest":"http://0.0.0.0:8080/v1"},"createTypes":{},"actions":{},"data":[{"id":"v1","type":"apiVersion","links":{"self":"http://0.0.0.0:8080/v1"},"actions":{}}],"sortLinks":{},"pagination":null,"sort":null,"filters":{},"createDefaults":{}}

alex-harvey-z3q · 2016-04-02T14:05:45Z

@petems

I set up the following test to determine whether Liam's code works:

describe command('date') do
  it {
    t = Time.now; puts "Sleeping at #{t}"; sleep 10
    o = subject.stdout; puts "subject.stdout is #{o}"
  }
end

The output:

  Command "date"
Sleeping at 2016-04-03 00:59:52 +1100

centos-72-x64 01:00:02$ /bin/sh -c date
  Sat Apr  2 14:00:01 UTC 2016

centos-72-x64 executed in 0.01 seconds
subject.stdout is Sat Apr  2 14:00:01 UTC 2016

Accordingly I believe I can write our test as:

context 'is expected to be running on the default port' do
  describe command('curl 0.0.0.0:8080') do
    it {
      sleep 5 # Give Rancher time to start.
      expect(subject.stdout).to match /self.*0.0.0.0:8080/
    }
  end
end

Is that OK with you?

petems · 2016-04-02T14:09:51Z

Hmm ok. Are you running the acceptance tests locally? It doesn't seem to work when I try.

alex-harvey-z3q · 2016-04-02T14:17:39Z

I'm running the tests on my laptop. Do you mean you tried that exact same code and got a different result?

petems · 2016-04-02T14:30:43Z

Ah, sorry about that, I misread, I just tried pulling from this PR and re-running it and still got the error. Didnt realise you'd not added in the updated code above yet! 😄

I'm just running it now with the updated code, it looks like it does work from your test, I must've written it wrong in my test or there's something different about my setup that causes the sleep not to work.

petems · 2016-04-02T14:35:25Z

Ok, just ran with Vagrant and still got the failure using the code from above:

context 'is expected to be running on the default port' do
      describe command('curl 0.0.0.0:8080') do
        it {
          sleep 30 # Give Rancher time to start.
          expect(subject.stdout).to match /self.*0.0.0.0:8080/
        }
      end
    end

alex-harvey-z3q · 2016-04-02T14:38:54Z

Perhaps you could add in the time-related puts statements to establish if it's failing because of timing or something else?

petems · 2016-04-02T14:55:25Z

I changed it to the following code:

    describe command('date') do
      it {
        t = Time.now; puts "Sleeping at #{t}"; sleep 10
        o = subject.stdout; puts "Before subject.stdout is #{o}"
      }
    end

    context 'is expected to be running on the default port' do
      describe command('curl 0.0.0.0:8080') do
        it {
          sleep 30 # Give Rancher time to start.
          expect(subject.stdout).to match /self.*0.0.0.0:8080/
        }
      end
    end

    describe command('date') do
      it {
        t = Time.now; puts "Sleeping at #{t}"; sleep 10
        o = subject.stdout; puts "After subject.stdout is #{o}"
      }
    end

    context 'is expected to be running on the default port (after the previous sleep)' do
      describe command('curl 0.0.0.0:8080') do
        it {
          sleep 30 # Give Rancher time to start.
          expect(subject.stdout).to match /self.*0.0.0.0:8080/
        }
      end
    end

And it passed successfully, but I think that's because the sleep happens in a separate rspec subject, and I'm not sure the benefit between doing the sleep within the machine or doing it as a sleep outside in a seperate block. It's your call, I'm cool with either as long as the test passes! 😄

petems · 2016-04-02T15:00:37Z

@alexharv074 To make things easier, I PR'd to add the ability to run the acceptance tests in Travis automatically (docker in docker in docker... inception) alex-harvey-z3q#1

When that's merged it should make proving the acceptance tests work easier 👍

alex-harvey-z3q · 2016-04-02T15:12:26Z

To be honest I'm having trouble getting it to work too but seems like it's another issue. I'm going to play around a bit more tomorrow (2.10am here in Sydney!). It's bugging me because I feel it's ugly to have a dummy test that just adds a sleep. I like rspec, and that kind of issue will cause me to dislike it! :)

petems · 2016-04-02T15:40:22Z

No worries! I don't think it's an issue with rspec tbh, probably with the way beaker-rspec handles the examples.

I also forgot to add in the new acceptance spec rake task, so just PR'd a new one: alex-harvey-z3q#2

See http://docs.rancher.com/rancher/rancher-ui/infrastructure/hosts/custom/

- more recent version of boilerplate. - fixes to get tests to pass.

alex-harvey-z3q · 2016-04-03T10:18:32Z

Ok, I timed it. It took 42 seconds for Rancher to start up. Now that I've set the wait time to 1 minute it seems fine.

alex-harvey-z3q · 2016-04-03T12:09:03Z

Ok, looks like the outstanding issue is the build's red because our new docker in docker in docker in Travis isn't working.

petems · 2016-04-03T13:43:54Z

spec/acceptance/node_spec.rb

+    it 'is expected to apply with no errors' do
+      pp = <<-EOS
+class { 'rancher::node':
+  'management'         => '10.0.1.1',


These don't need single quotes, that's what's causing the syntax error should be:

class { '::rancher::node': management => '10.0.1.1', registration_token => 'DB121CFBA836F9493653:1434085200000:2ZOwUMd6fIzz44efikGhBP1veo', }

alex-harvey-z3q · 2016-04-03T14:25:24Z

.travis.yml

+    env: PUPPET_GEM_VERSION='~> 4.3.0' STRICT_VARIABLES=yes
+  - rvm: 2.1.6
+    env: PUPPET_GEM_VERSION='~> 4.0' STRICT_VARIABLES=yes
+  - rvm: 2.1


2.1 or 2.1.6?

alex-harvey-z3q · 2016-04-03T14:51:30Z

Now the build is failing with:

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
The build has been terminated

Investigating ways to raise the time out.

petems · 2016-04-03T17:22:40Z

tbh, it's possible that you can't run docker in docker in docker inside travis instances...

I say, leave the acceptance tests in for people to run manually to confirm things are working, remove them from the travis.yml and get this merged in. We can investigate the feasibility of doing the acceptance tests as part of Travis afterwards 👍

nickschuch · 2016-04-03T23:12:01Z

I agree. This is WAY more than what we had before, happy to see the acceptance tests get debugged in a seperate PR.

alex-harvey-z3q · 2016-04-04T08:55:54Z

I also agree, and feel free to merge.

alex-harvey-z3q reviewed Mar 31, 2016
View reviewed changes

petems reviewed Mar 31, 2016
View reviewed changes

alex-harvey-z3q force-pushed the refactor_and_add_tests branch from 86105d2 to c3a632f Compare April 1, 2016 03:58

petems reviewed Apr 1, 2016
View reviewed changes

alex-harvey-z3q referenced this pull request in mizzy/serverspec Apr 1, 2016

Another implementaion of Docker resource type (Ref #480)

ac74ce5

Update Boilerplate for testing

e6c90b0

petems and others added 11 commits April 3, 2016 17:45

Updates to use the latest agent setup from docs

831839c

See http://docs.rancher.com/rancher/rancher-ui/infrastructure/hosts/custom/

Change to use docker class rather than package

5d226d5

Allow setting port that Rancher master runs on

78d05cb

Move acceptance specs to standard location.

47dab19

Refactor tests.

5b628fe

- more recent version of boilerplate. - fixes to get tests to pass.

Remove Gemfile.lock from git.

fcc245a

Remove class_spec.rb

5294540

Use latest CentOS 7.2 in Beaker nodesets.

3cb8901

Updates .travis.yml.

66a9ae5

JQ formatted metadata.json.

05a6c10

Compilation should fail if rancher::node is passed invalid input.

1052fe9

alex-harvey-z3q force-pushed the refactor_and_add_tests branch from ef58162 to 08ef1d0 Compare April 3, 2016 07:53

petems reviewed Apr 3, 2016
View reviewed changes

alex-harvey-z3q added 2 commits April 4, 2016 00:09

Add some missing facts.

cb93bae

Add working acceptance tests.

ea1d061

alex-harvey-z3q force-pushed the refactor_and_add_tests branch from d4afb8e to 4edef77 Compare April 3, 2016 14:10

alex-harvey-z3q reviewed Apr 3, 2016
View reviewed changes

petems mentioned this pull request Apr 3, 2016

Refactor and add tests #5

Closed

alex-harvey-z3q force-pushed the refactor_and_add_tests branch from 43ae91d to ea1d061 Compare April 4, 2016 03:26

alex-harvey-z3q added 3 commits April 4, 2016 13:33

Minor changes for style consistency.

7a802b5

Resolve metadata take task deprecation warning.

e186d20

Update README with test instructions.

e272525

nickschuch merged commit 034c0bd into nickschuch:master Apr 4, 2016

alex-harvey-z3q deleted the refactor_and_add_tests branch April 4, 2016 12:06

Refactor and add tests #6

Refactor and add tests #6

Conversation

alex-harvey-z3q commented Mar 31, 2016

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

ozbillwang commented Mar 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petems commented Mar 31, 2016

alex-harvey-z3q commented Apr 1, 2016

alex-harvey-z3q commented Apr 1, 2016

alex-harvey-z3q commented Apr 1, 2016

alex-harvey-z3q commented Apr 1, 2016

alex-harvey-z3q commented Apr 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-harvey-z3q commented Apr 1, 2016

petems commented Apr 1, 2016

alex-harvey-z3q commented Apr 2, 2016

petems commented Apr 2, 2016

alex-harvey-z3q commented Apr 2, 2016

petems commented Apr 2, 2016

petems commented Apr 2, 2016

alex-harvey-z3q commented Apr 2, 2016

petems commented Apr 2, 2016

petems commented Apr 2, 2016

alex-harvey-z3q commented Apr 2, 2016

petems commented Apr 2, 2016

alex-harvey-z3q commented Apr 3, 2016

alex-harvey-z3q commented Apr 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-harvey-z3q commented Apr 3, 2016

petems commented Apr 3, 2016

nickschuch commented Apr 3, 2016

alex-harvey-z3q commented Apr 4, 2016