Integration tests and helpers #24

KnVerey · 2017-02-18T20:48:10Z

This PR adds coverage for most of what I was looking for in the manual tophat flow with the "trashbin" repo, plus a couple bonus scenarios. Not covered:

Error logging related to deploys that eventually time out. The current behaviour would be hard to integration test (could be covered by unit tests on the polling) and switching these to be fast failures is near the top of my todo list.
The actual CLI invocation itself (tests trigger deploys via the Runner)

The most interesting part here was trying to devise a helper structure would make these tests easy to read, simple to write and clean to customize. My idea of what this would look like definitely changed as I wrote more tests! What I landed on here is perhaps a bit unusual, but I quite like the API it gives the tests themselves, and the encapsulation of concerns in the helpers. I recommend checking out the integration test file first, since it is the whole point of the rest of the code.

Details

Here's an overview of how the helpers are set up:

FixtureDeployHelper contains methods for loading and manipulating the yaml fixtures. It provides two options:
- deploy_fixture_set(set, subset=nil, wait: true) to immediately deploy a full or partial set of fixtures
- load_fixture_data(set, subset=nil) + deploy_loaded_fixture_set(template_map, wait: true) allows you to make changes/additions to the set before deploying it. The alternative to providing this option is to duplicate the fixtures for each scenario that needs a change. I find this much better because it not only reduces duplication, but also makes it super clear within the test what is special (aka broken in most cases) about the set being deployed.
FixtureSetAssertions::Basic is used to encapsulate knowledge about the contents of the fixture set named "basic" (I'm kinda regretting that name... too adjectival). You instantiate it with the namespace you deployed to, then use it to assert things like "the whole set is up", "all the redis-related components are up", or "no web-related components exist".
FixtureSetAssertions::FixtureSet is the superclass of FixtureSetAssertions::Basic and provides generic resource assertions that can be leveraged by any fixture set class (e.g. assert_service_up(svc_name)). I made these methods private, but it might make sense for tests to be able to use them directly as well, i.e. when they've added something to the set deployed.

kirs

Great work and lots of checks/assertions 👍
I like how you cleaned up the tests and moved assertions into separate reusable methods.

The PR looks good overall, except a few things I've noted about the code structure.

kirs · 2017-02-22T18:36:40Z

test/helpers/fixture_deploy_helper.rb

@@ -0,0 +1,90 @@
+module FixtureDeployHelper
+  # use deploy_fixture_set if you are not adding to or otherwise modifying the template set
+  def deploy_fixture_set(set, subset=nil, wait: true)


worth making subset a keyword argument as well?

kirs · 2017-02-22T18:39:14Z

test/helpers/fixture_deploy_helper.rb

+    runner.run
+  end
+
+  # HELPER METHODS BELOW NOT INTENDED FOR INCLUDING CLASS USE


Why not making them private?

hmm, good question. 😆
This probably came from playing with moving things around... earlier on there were a bunch of different types of things in the same file and I had notes all over the place. Will fix!

kirs · 2017-02-22T18:43:55Z

test/helpers/fixture_deploy_helper.rb

+    end
+    deploy_dir(dir, wait: wait)
+  ensure
+    files.each { |f| File.delete(f) }


can't we just remove the dir entirely?

yep, definitely!

kirs · 2017-02-22T18:44:57Z

test/helpers/fixture_deploy_helper.rb

+    files.each { |f| File.delete(f) }
+  end
+
+  # load_fixture_data return format


You can make this the RDoc compatible example.

kirs · 2017-02-22T19:04:11Z

test/integration/kubernetes_deploy_test.rb

-      pod.metadata.ownerReferences && pod.metadata.ownerReferences.first.kind == "ReplicaSet"
+  def test_full_basic_set_deploy_succeeds
+    deploy_fixture_set("basic")
+    basic = FixtureSetAssertions::Basic.new(@namespace)


What's the value of wrapping assertions into FixtureSetAssertions::Basic.new if we can use assert_all_up(@namespace) as a module method, and include this assertions module into KubernetesDeploy::IntegrationTest?

It'd have to be assert_all_up('basic', @namespace) or assert_all_basic_up(@namespace), unless we consider it unlikely there'll be multiple fixture sets (hard to say--there's a second now, but we don't need any assertions for it so far). In any case, I actually started out with the latter and moved to this class-based structure as an experiment, and kept it because I found it cleaner and liked the more forceful encapsulation of the knowledge of what the fixtures contain. I'm open to switching back if you guys hate this. 😄

kirs · 2017-02-22T19:23:34Z

test/helpers/fixture_deploy_helper.rb

+
+  def each_k8s_yaml(source_dir, subset)
+    Dir["#{source_dir}/*.yml*"].each do |filename|
+      match_data = File.basename(filename).match(/(?<basename>.*)(?<ext>\.yml(?:\.erb)?)\z/)


If what we want to extract here is basename and extname, you can use File.basename and File.extname.

What we need to extract is:

/foo/bar/baz/template.yml -> 'template' and '.yml'

/foo/bar/baz/template.yml.erb -> 'template' and '.yml.erb'

The problem I was having with just using those two is that File.extname only grabs the last extension, and File.basename needs you to say exactly which extension you're removing, whereas there are two options.

it'a usually more reliable to use standard library for such things because it eliminates cases not covered by the regexp, but here I see why it wouldn't work 👍

An alternative would be:

parts = File.basename(filename).split('.') basename, ext = parts[0], parts[1..-1]

kirs · 2017-02-22T19:24:58Z

test/helpers/fixture_deploy_helper.rb

+    files.each { |f| File.delete(f) } if files
+  end
+
+  # use load_fixture_data + deploy_loaded_fixture_set to have the chance to add to / modify the template set before deploy


May be worth it converting this into RDoc with a usage example (like what Rails does)

I'm happy to do this, but I haven't written RDoc comments before and I'm having trouble finding the standards for usage examples. Can you point me in the right direction tomorrow?

Here is an example: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/core.rb#L266-L271

Those comments in that file don't look super standardized to me, so I'm still not really sure what you want to see. I'll take a stab at it though.

kirs · 2017-02-22T19:25:17Z

test/fixtures/invalid/yaml-error.yml

+    name: basic-configmap-data
+    app: basic
+data:
+  datapoint1: value1:


is it intended to be value1:?

yes, that's the yaml error :)

kirs · 2017-02-22T19:25:48Z

test/test_helper.rb

@@ -77,4 +82,5 @@ def self.prepare_pv(name)
  end

  TestProvisioner.prepare_pv("pv0001")
+  TestProvisioner.prepare_pv("pv0002")


Is pv0002 used somewhere else or you created it to allocate more space?

If I understand correctly how this kind of PV works (as opposed to the dynamic kind you get via storage classes), the entire PV is reserved by PVCs made against it. So in theory pv0001 could still be getting recycled from a previous test/namespace's claim the next time we need it. I didn't actually see this happen though.

kirs · 2017-02-22T19:40:13Z

test/helpers/fixture_sets/basic.rb

@@ -0,0 +1,63 @@
+module FixtureSetAssertions
+  class Basic < FixtureSet


TBH I'm not a big fan of naming it Basic in this case, because first time when you look at it you think it's a base class like ActiveRecord::Base, but then you see it's only about the basic scenario.

As an alternative I can think about moving basic scenario testing into test/integration/basic_test.rb and storing all basic helpers right in that file together with tests. This way you can limit basic assertions only to that file.

IMO that still sounds like some sort of "basic" test rather than a group of tests that all run against a fixture called "basic". How about we ignore the fixture name in choosing the structure, and rename the fixture in a follow-up? Maybe even a suffix like "App" would help. Or perhaps "HelloWorld" or "HelloCloud", or something outlandish. I dunno.

Great idea! I like HelloCloud 👍

HelloCloud 👋 ☁️

I don't have a strong opinion on leaving the helpers here or in the integration tests. It certainly helps break up things later, I think what that comes down to is how many fixture sets do we expect to have? Do you expect 2? 5? 10? I think that would also impact some of the other work in this PR if we don't really expect to ever need more than HelloCloud. I think you definitely made the right call to make it easy to manipulate the set instead of creating a lot of new ones, but perhaps people will find that so convenient they won't create sets altogether. Do you see any use-case for having more than one fixture set?

how many fixture sets do we expect to have? Do you expect 2? 5? 10? Do you see any use-case for having more than one fixture set?

I'd expect 3-5, but I'm really just speculating. We technically have a second in this PR; it contains a template with yaml errors (afaik not possible to create by dumping a ruby structure).

Two main possibilities come to mind:

Shopify's closed-source ThirdPartyResources that the script explicitly supports. There are two today, and likely to be several more in the future. Having a separate "shopify" set for them could make sense. That said, these tests would be questionable to set up, as we'd need to have the minikube cluster running the actual TPR controllers or the deploys won't succeed.

Sets that are templated differently, if/when we finally move forward with changing the templating engine and supporting partials + a top-level "values" yaml.

kirs · 2017-02-22T19:42:41Z

sorry, I meant to choose this one not the approval

sirupsen

I haven't reviewed the test coverage in detail but in general I am liking the direction. I really love the dedication for writing good tests for this @KnVerey and @kirs.. this is going to pay off so many times in the coming years.

sirupsen · 2017-02-24T11:17:53Z

lib/kubernetes-deploy/runner.rb

@@ -319,7 +319,7 @@ def phase_heading(phase_name)
    end

    def log_green(msg)
-      STDOUT.puts "\033[0;32m#{msg}\x1b[0m\n" # green
+      KubernetesDeploy.logger.info("\033[0;32m#{msg}\x1b[0m")


I would consider writing a helper for the colors or using a gem. Burke once gave me an amazing rule of thumb: "Start at 31. Ordered by usefulness (red, green, yellow)." as a great way of remembering them.. but most people haven't been subject to review 100s of Burke's bash PRs. 😄

Added this to the master issue under "code" (this change is actually from the branch this is based on, which I already merged, and will disappear when I push the rebase).

sirupsen · 2017-02-24T11:44:22Z

test/helpers/fixture_deploy_helper.rb

@@ -0,0 +1,90 @@
+module FixtureDeployHelper
+  # use deploy_fixture_set if you are not adding to or otherwise modifying the template set
+  def deploy_fixture_set(set, subset: nil, wait: true)


Could this method simply be load_fixture_data + deploy_loaded_fixture_set? This is obviously a more efficient implementation since you have to do less work when you know you can deploy the pure sets, however, I think simplicity could trump efficiency here

Besides the efficiency aspect, I'd have one concern about that change: deploy_loaded_fixture_set assumes that there's ERB in the file (because you could have added some, even if there wasn't any to begin with), so if we always went through it, we'd fail to exercise the script's recognition of pure .yml files. Removing that assumption from deploy_loaded_fixture_set would require exposing the original extension somewhere in the loaded fixture hash and likely adding a bit of complexity to deploy_loaded_fixture_set, so there's a bit of a tradeoff. I could go either way, but I slightly favour the way it is now.

sirupsen · 2017-02-24T11:47:59Z

test/helpers/fixture_deploy_helper.rb

+  # load_fixture_data return format
+  # {
+  #   file_basename: {
+  #     type_name: [


I ❤️ comments like this that give me an overview of the data that's being transformed, and what it's being transformed into. My initial question here was why you didn't go for a simpler structure of an array of hashes, but it's clear that there are 2 consumers of this: (1) tests that want to mutate / inspect this data, and (2) deploy_loaded_fixture_set. You need this formatting for (1), not for (2), which makes sense to me 👍 If the comment reflected example of an actual filename and a couple of definitions in there, that'd make this comment even more helpful I think.

sirupsen · 2017-02-24T11:48:37Z

test/helpers/fixture_deploy_helper.rb

+
+  def fixture_path(set_name)
+    source_dir = File.expand_path("../../fixtures/#{set_name}", __FILE__)
+    raise "Fixture set pat #{source_dir} is invalid" unless File.directory?(source_dir)


Insignificant typo 🚓

Fixture set #{set_name} does not exist as a directory in: #{source_dir}.

sirupsen · 2017-02-24T11:50:20Z

test/helpers/fixture_deploy_helper.rb

+    templates
+  end
+
+  def deploy_dir(dir, sha: 'abcabcabc', wait: true)


Can this also be private?

sirupsen · 2017-02-24T12:16:39Z

test/helpers/fixture_sets/basic.rb

@@ -0,0 +1,63 @@
+module FixtureSetAssertions
+  class Basic < FixtureSet


I don't have a strong opinion on leaving the helpers here or in the integration tests. It certainly helps break up things later, I think what that comes down to is how many fixture sets do we expect to have? Do you expect 2? 5? 10? I think that would also impact some of the other work in this PR if we don't really expect to ever need more than HelloCloud. I think you definitely made the right call to make it easy to manipulate the set instead of creating a lot of new ones, but perhaps people will find that so convenient they won't create sets altogether. Do you see any use-case for having more than one fixture set?

sirupsen · 2017-02-24T12:18:15Z

test/integration/kubernetes_deploy_test.rb

+    pods = kubeclient.get_pods(namespace: @namespace, label_selector: "name=web,app=basic")
+    running_pods, not_running_pods = pods.partition { |pod| pod.status.phase == "Running" }
+    assert_equal 1, running_pods.size
+    assert not_running_pods.size >= 1


Would this be better as a helper?

I don't think so, unless I'm misunderstanding for what... basic.assert_some_web_pods_running(running: N, min_not_running: M) doesn't strike me as clearer or reusable.

sirupsen · 2017-02-24T12:18:30Z

test/integration/kubernetes_deploy_test.rb

+    end
+    assert_match /Dry run failed for template configmap-data/, error.to_s
+
+    @logger_stream.rewind


It's very likely someone will forget to do this, can we use a shared helper?

sirupsen · 2017-02-24T12:20:47Z

test/integration/kubernetes_deploy_test.rb

+    error = assert_raises(KubernetesDeploy::FatalDeploymentError) do
+      deploy_fixture_set("invalid", subset: ["yaml-error"])
+    end
+    assert_match /Template \S+ cannot be parsed/, error.to_s


I think you can use assert_raises with a regex as the second argument here to assert on the message.

error_message = "/Template \S+ cannot be parsed/" assert_raises(KubernetesDeploy::FatalDeploymentError, error_message) { .. }

sirupsen · 2017-02-24T12:21:12Z

test/integration/kubernetes_deploy_test.rb

    end
+    assert_match /1 priority resources failed to deploy/, error.to_s


Do we want to assert that something about the image not being found is in the logging output?

We can do assert_logs_match(/DeadlineExceeded/), but note that there probably won't be anything about the bad image in the logs. A one-second deadline is not likely enough for the image pull attempt to finish in the first place; the activeDeadlineSeconds is the fastest way to produce these circumstances, and the bad image is just to make sure the pod won't flakily succeed if ever it does manage to finish pulling.

sirupsen

I really like the shape this is in now @KnVerey. Way to go ✨

sirupsen · 2017-02-27T11:19:01Z

test/helpers/fixture_deploy_helper.rb

@@ -0,0 +1,92 @@
+module FixtureDeployHelper
+  # Deploys the specified set of fixtures via KubernetesDeploy::Runner.


This is a really awesome API Katrina. Great job.

sirupsen · 2017-02-27T11:21:56Z

test/integration/kubernetes_deploy_test.rb

+
+  def test_invalid_yaml_fails_fast
+    assert_raises(KubernetesDeploy::FatalDeploymentError, /Template \S+yaml-error\S+ cannot be parsed/) do
+      deploy_dir(fixture_path("invalid"))


Since this is the only test using this, do we need to expose it? Is this for the case where the fixture path has no ERB files? I think we can just rely on them always being ERB, or make sure that whatever reads them can understand ERB and non-ERB.

The purpose of this test is to cover deploy behaviour in the case where invalid yaml is provided. It needs circumvent the loading of the yaml file into a ruby structure, or else the yaml parse exception will be raised in the setup helpers instead of during the deploy. So we can't get it to use deploy_fixtures, but I can have it explicitly use the KubernetesDeploy::Runner instead of exposing the helper if you prefer.

kirs · 2017-02-27T23:31:12Z

test/helpers/fixture_deploy_helper.rb

+  # # The following will deploy the "basic" fixture set, but with the unmanaged pod modified to use a bad image
+  #   deploy_fixtures("basic") do |fixtures|
+  #     pod = fixtures["unmanaged-pod.yml.erb"]["Pod"].first
+  #     pod["spec"]["containers"].first["image"] = "hello-world:thisImageIsBad"


Thanks for making it a great doc!

kirs · 2017-02-27T23:33:48Z

test/helpers/fixture_set.rb

+      raise NotImplementedError
+    end
+
+    def assertions


This is required to mimic minitest assertions, right?

Exactly. See minitest/minitest#286.

kirs · 2017-02-27T23:34:50Z

test/helpers/fixture_set.rb

+      resources = client.public_send("get_#{type}", name, namespace) # 404s
+      flunk "#{type} #{name} unexpectedly existed"
+    rescue KubeException => e
+      raise unless e.to_s.include?("not found")


Looks like it would be great for a gem to provide KubeNotFoundError specific for 404.

Yes, I wish. Maybe we can dig in and PR that upstream.

kirs · 2017-02-27T23:35:47Z

test/helpers/fixture_set.rb

+
+    def assert_configmap_present(cm_name, expected_data)
+      configmaps = kubeclient.get_config_maps(namespace: namespace, label_selector: "name=#{cm_name},app=#{app_name}")
+      assert_equal 1, configmaps.size, "Expected 1 configmap, got #{configmaps.size}"


👍 for passing the error message

kirs · 2017-02-27T23:37:01Z

test/test_helper.rb

@@ -19,10 +23,16 @@ def setup
    def teardown
      @logger_stream.close
    end
+
+    def assert_logs_match(regexp)


kirs

Really love the shape of the code after rounds of the code review. Thanks for working on it!

KnVerey requested review from sirupsen, kirs and ibawt February 18, 2017 20:48

kirs approved these changes Feb 22, 2017

View reviewed changes

KnVerey force-pushed the tests3 branch from 7cc7b7a to c790553 Compare February 23, 2017 00:44

KnVerey force-pushed the fix_loggers branch from 198afad to f14068d Compare February 23, 2017 01:18

sirupsen reviewed Feb 24, 2017

View reviewed changes

KnVerey added 7 commits February 24, 2017 22:02

Add some tests and handy helpers

19253e3

Regression test for evicted pod problem

24e700c

Code review fixes

579b535

Clearer bad image name

f6048ea

Improve comments

5afba98

Misc other PR feedback fixes

2aa1fce

Use same API for deploys of defaults and modified fixtures

f254f44

KnVerey force-pushed the tests3 branch from c790553 to f254f44 Compare February 25, 2017 03:05

KnVerey changed the base branch from fix_loggers to master February 25, 2017 03:05

KnVerey mentioned this pull request Feb 27, 2017

Try to find a way around having two kubeclient helpers in tests #51

Closed

sirupsen approved these changes Feb 27, 2017

View reviewed changes

kirs reviewed Feb 27, 2017

View reviewed changes

test/test_helper.rb

@@ -19,10 +23,16 @@ def setup

def teardown

@logger_stream.close

end

def assert_logs_match(regexp)

Copy link

Contributor

kirs Feb 27, 2017

Choose a reason for hiding this comment

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

😍

kirs approved these changes Feb 27, 2017

View reviewed changes

KnVerey merged commit abd37e7 into master Feb 27, 2017

KnVerey deleted the tests3 branch February 27, 2017 23:44

KnVerey mentioned this pull request Apr 21, 2017

Fix assertions on exception messages #82

Merged

KnVerey mentioned this pull request May 31, 2017

Rework deploy output #98

Merged

3 tasks

		@@ -0,0 +1,63 @@
		module FixtureSetAssertions
		class Basic < FixtureSet

		end
		assert_match /1 priority resources failed to deploy/, error.to_s

		@@ -0,0 +1,92 @@
		module FixtureDeployHelper
		# Deploys the specified set of fixtures via KubernetesDeploy::Runner.

Integration tests and helpers #24

Integration tests and helpers #24

Conversation

KnVerey commented Feb 18, 2017

Details

kirs 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

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

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

Choose a reason for hiding this comment

kirs commented Feb 22, 2017

sirupsen 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

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

sirupsen 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirs left a comment

Choose a reason for hiding this comment