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

Integration tests and helpers #24

Merged
merged 7 commits into from
Feb 27, 2017
Merged

Integration tests and helpers #24

merged 7 commits into from
Feb 27, 2017

Conversation

KnVerey
Copy link
Contributor

@KnVerey KnVerey commented Feb 18, 2017

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.

@KnVerey KnVerey requested review from sirupsen, kirs and ibawt February 18, 2017 20:48
Copy link
Contributor

@kirs kirs left a comment

Choose a reason for hiding this comment

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

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.

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

worth making subset a keyword argument as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure 👍

runner.run
end

# HELPER METHODS BELOW NOT INTENDED FOR INCLUDING CLASS USE
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not making them private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

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

Choose a reason for hiding this comment

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

can't we just remove the dir entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, definitely!

files.each { |f| File.delete(f) }
end

# load_fixture_data return format
Copy link
Contributor

Choose a reason for hiding this comment

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

You can make this the RDoc compatible example.

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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. 😄


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/)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍

Choose a reason for hiding this comment

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

An alternative would be:

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

name: basic-configmap-data
app: basic
data:
datapoint1: value1:
Copy link
Contributor

Choose a reason for hiding this comment

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

is it intended to be value1:?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that's the yaml error :)

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

TestProvisioner.prepare_pv("pv0001")
TestProvisioner.prepare_pv("pv0002")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great idea! I like HelloCloud 👍

Choose a reason for hiding this comment

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

HelloCloud 👋 ☁️

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

kirs commented Feb 22, 2017

sorry, I meant to choose this one not the approval

screen shot 2017-02-22 at 14 42 13

Copy link

@sirupsen sirupsen left a comment

Choose a reason for hiding this comment

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

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.

@@ -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")

Choose a reason for hiding this comment

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

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. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

@@ -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)

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.


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)

Choose a reason for hiding this comment

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

Insignificant typo 🚓

Choose a reason for hiding this comment

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

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

templates
end

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

Choose a reason for hiding this comment

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

Can this also be private?

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

Choose a reason for hiding this comment

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

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?

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

Choose a reason for hiding this comment

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

Would this be better as a helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

end
assert_match /Dry run failed for template configmap-data/, error.to_s

@logger_stream.rewind

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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) { .. }

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

@sirupsen sirupsen left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

This is a really awesome API Katrina. Great job.


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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

# # 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"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for making it a great doc!

raise NotImplementedError
end

def assertions
Copy link
Contributor

Choose a reason for hiding this comment

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

This is required to mimic minitest assertions, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. See minitest/minitest#286.

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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


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}"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for passing the error message

@@ -19,10 +23,16 @@ def setup
def teardown
@logger_stream.close
end

def assert_logs_match(regexp)
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

Copy link
Contributor

@kirs kirs left a comment

Choose a reason for hiding this comment

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

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

@KnVerey KnVerey merged commit abd37e7 into master Feb 27, 2017
@KnVerey KnVerey deleted the tests3 branch February 27, 2017 23:44
@KnVerey KnVerey mentioned this pull request May 31, 2017
3 tasks
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.

3 participants