split bats into the real tests and roles to set stuff up #465

evgeni · 2017-06-02T15:32:55Z

before we had bats doing a lot of setup and then running tests.
this moves the setup tasks into a series of own roles (haveged, disable_firewall, clean_puppet) and lets bats do what it is supposed to do: run tests.

stbenjam

Looks good to me

ekohl · 2017-06-02T18:09:24Z

boxes.yaml.example

@@ -44,6 +44,7 @@ centos7-bats-ci:
      bats_tests:
        - "swapfile.bats"
        - "fb-install-katello.bats"


IMHO this can be left out now. We should rely on ansible for installation.

You mean basically completely dropping fb-install-katello.bats? Yeah we can do that.

The file can stay, but we shouldn't recommend using it I think.

If the file is not used and not recommended, why should it stay? Archaeology lessons can still be done using the Git history, an unused file is just confusing. Same applies for katello-bats btw, I think it's unused by now?

That's the problem I have with this repo: I don't know how people use it. Maybe @ehelms or @stbenjam know it better.

One of the biggest things we need is https://github.com/evgeni/forklift/blob/99fdda8d44631373e75ece1bb3aefdc2a970f52a/bats/fb-install-katello.bats#L47-L52 which was put in to help out on the CI system as far as I remember.

So maybe we need like a setup role to do a few of those items (instead of "testing" them). Otherwise I think it's safe to remove the install test file.

For now I'd suggest a haveged role rather than a more global setup role which may get parameters. Start simple.

ekohl · 2017-06-02T18:09:34Z

playbooks/roles/bats/defaults/main.yaml

@@ -5,5 +5,6 @@ bats_output_dir: "/root/bats_results"
 bats_update_forklift: "yes"
 bats_tests:
  - "fb-install-katello.bats"


Should also be left out IMHO.

ekohl · 2017-06-02T18:10:02Z

Thanks you for this! I wanted to do this but hadn't gotten around to it.

ehelms · 2017-06-07T14:34:47Z

bats/fb-test-katello.bats

@@ -0,0 +1,42 @@
+#!/usr/bin/env bats


@evgeni I think one additional check could be valuable here -- doing a curl of /katello/api/ping since this provides information of about services that the status check does not, e.g. whether or not Katello can talk to Pulp or Candlepin and whether or not Pulp is having any connectivity issues with the workers. These are things that can produce big smells for installation health.

That's what hammer ping does. No need to curl yourself ;)

ehelms · 2017-06-07T14:48:15Z

bats/fb-test-katello.bats

+}
+
+@test "check host is registered" {
+  [ x$FOREMAN_VERSION = "x1.3" ] && skip "Only supported on 1.4+"


Ha - I think we could drop this line

So I did in my local branch, let me polish and push that before you continue review.

evgeni · 2017-06-07T15:21:36Z

@ekohl @ehelms @stbenjam please re-review, I made all the setup steps into own roles, and killed fb-install-katello.bats :)

ehelms · 2017-06-07T17:03:09Z

Looks good to me, will let @ekohl have a final pass at the updated version

ekohl · 2017-06-07T19:03:49Z

bats/fb-test-katello.bats

+
+@test "check smart proxy is registered" {
+  count=$(hammer --csv proxy list | wc -l)
+  [ $count -gt 1 ]


I wonder if grepping for the hostname would be a better check, but I think this is directly ported so it's fine to leave this in.

yeah. grep -q $hostname sounds good to me.

Sent from my OnePlus ONEPLUS A3003 using FastHub

ekohl · 2017-06-07T19:06:12Z

playbooks/roles/clean_puppet/tasks/main.yml

@@ -0,0 +1,22 @@
+---


I'd drop this role. It was originally added in the foreman-bats because you want to start with a clean install, but in the ansible context it doesn't really make sense (unless you do it before the foreman-installer run).

ack. Was not sure about the history and thought porting was cheaper then thinking.

ekohl · 2017-06-07T19:07:34Z

playbooks/roles/haveged/tasks/main.yml

+- name: 'Install Haveged (faster population of PRNG)'
+  yum:
+    name: haveged
+    state: latest


I think just ensuring it's installed is good enough.

Thats how it was in the freeipa role.

Sent from my OnePlus ONEPLUS A3003 using FastHub

evgeni · 2017-06-08T07:09:41Z

@ekohl there we go, all 3 comments done

before we had bats doing a lot of setup and then running tests. this moves the setup tasks into a series of own roles (haveged, disable_firewall) and lets bats do what it is supposed to do: run tests.

ekohl

Untested but it looks correct.

ekohl · 2017-06-09T13:30:15Z

Thanks!

) before we had bats doing a lot of setup and then running tests. this moves the setup tasks into a series of own roles (haveged, disable_firewall) and lets bats do what it is supposed to do: run tests.

stbenjam approved these changes Jun 2, 2017

View reviewed changes

ekohl requested changes Jun 2, 2017

View reviewed changes

ehelms reviewed Jun 7, 2017

View reviewed changes

evgeni force-pushed the modular-bats branch from 99fdda8 to 32de9f6 Compare June 7, 2017 15:19

evgeni changed the title ~~move the basic katello tests to an own bats file~~ split bats into the real tests and roles to set stuff up Jun 7, 2017

ehelms approved these changes Jun 7, 2017

View reviewed changes

ekohl requested changes Jun 7, 2017

View reviewed changes

evgeni force-pushed the modular-bats branch from 32de9f6 to dfd9173 Compare June 8, 2017 07:07

split bats into the real tests and roles to set stuff up

bbbde8b

before we had bats doing a lot of setup and then running tests. this moves the setup tasks into a series of own roles (haveged, disable_firewall) and lets bats do what it is supposed to do: run tests.

evgeni force-pushed the modular-bats branch from dfd9173 to bbbde8b Compare June 8, 2017 10:29

ekohl approved these changes Jun 8, 2017

View reviewed changes

ekohl merged commit 1619743 into theforeman:master Jun 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

split bats into the real tests and roles to set stuff up #465

split bats into the real tests and roles to set stuff up #465

evgeni commented Jun 2, 2017 •

edited

stbenjam left a comment

ekohl Jun 2, 2017

evgeni Jun 2, 2017

ekohl Jun 2, 2017

evgeni Jun 3, 2017

ekohl Jun 3, 2017

ehelms Jun 5, 2017

ekohl Jun 5, 2017

ekohl Jun 2, 2017

ekohl commented Jun 2, 2017

ehelms Jun 7, 2017

evgeni Jun 7, 2017

ehelms Jun 7, 2017

evgeni Jun 7, 2017

evgeni commented Jun 7, 2017

ehelms commented Jun 7, 2017

ekohl Jun 7, 2017

evgeni Jun 7, 2017

ekohl Jun 7, 2017

evgeni Jun 8, 2017

ekohl Jun 7, 2017

evgeni Jun 7, 2017

evgeni commented Jun 8, 2017

ekohl left a comment

ekohl commented Jun 9, 2017

split bats into the real tests and roles to set stuff up #465

split bats into the real tests and roles to set stuff up #465

Conversation

evgeni commented Jun 2, 2017 • edited

stbenjam left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Jun 2, 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

evgeni commented Jun 7, 2017

ehelms commented Jun 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

evgeni commented Jun 8, 2017

ekohl left a comment

Choose a reason for hiding this comment

ekohl commented Jun 9, 2017

evgeni commented Jun 2, 2017 •

edited