Add integration tests for InstanceController

[gkleiman]

What type of PR is this?

/kind enhancement

What this PR does / why we need it:
This patch adds integration tests for InstanceController, which will enable us to ensure that we don't change the existing behavior when cleaning it up and when working on #422.

Does this PR introduce a user-facing change?:

NONE

[oliviabarrick]

This looks great!

Some thoughts:

• We should decide on how we want to document our tests. We could do either a README.md in each test case’s directory or comments in the file or something more structured. That is probably a decision for outside of this PR/a KEP, though.
• I should update the harness to allow merge patching in the updates to make these more DRY.

[alenkacz]

Looks good!

Just one note - I think this should be e2e tests not integration tests. We already have integration tests for controllers and it's different level that this one - it's confusing then.

As much as I like the simplicity of the harness, there are things that worry me. In general I am just worried that all this is not code so it will be real pain to do any kind of refactoring in that :( Like lots of the strings are hardcoded... We'll see :) I am just not a big fan of maintaining a lot of YAML. Also to see what are the steps actually doing you have to read A LOT of yaml across several files. So to get some overview, it's also pretty hard.

[oliviabarrick]

@alenkacz Im definitely all ears on suggestions on how we can improve it.

[oliviabarrick]

Also these are integration tests: https://github.com/kudobuilder/kudo/blob/master/keps/0004-add-testing-infrastructure.md#integration-tests

We could change that on the KEP, but I think the tests you’re talking about are more likely to be unit tests - or just also integration tests.

[zen-dog]

Nice work. Just had one question about the TestStep

[zen-dog]

What's a TestStep? A debug remnant?

[gkleiman]

Here's Justin's definition of TestStep from the corresponding KEP:

When searching a test step file, if a TestStep object is found, it includes settings to apply to the step. This object is not required - if it is not specified then defaults are used. No more than one TestStep object should be defined in a given test step.

In this case I'm using the TestStep object to make the test harness delete the first deploy PlanExecution.

[zen-dog]

Got ya!

[gkleiman]

Changing the name of the PR back to Add integration tests for InstanceController since the KEP describes integration tests and the tests are under test/integration =).

[zen-dog]

As much as I like the simplicity of the harness, there are things that worry me. In general I am just worried that all this is not code so it will be real pain to do any kind of refactoring in that :( Like lots of the strings are hardcoded...

I echo that but I guess we'll see how maintainable that will become. In the meantime, it is somewhat hard to identify/follow what behavior is supposed to be tested. I'm missing something like Scala's Given/When/Then/And:

Given("an an Intance with parameter 'foo' and no 'triggerPlan'")
...
When("the parameter foo is updatet")
...
Then("a default execution plan 'deploy' should be created")
...
And("...something else?")

So maybe we could put these as YAML comments for now? Wdyt @gkleiman , @alenkacz , @justinbarrick ?

[jbarrick-mesosphere]

@zen-dog yeah, I think YAML comments are the way to go at the moment.

I'm not sure what you're hoping for with the Given/When/Then/And thing, just documentation? We do have these in practice when you define your steps and asserts, it's just a matter of how we want to structure and document them.

[zen-dog]

I'm not sure what you're hoping for with the Given/When/Then/And thing, just documentation?

That and better understanding of individual steps and asserts. E.g. here, we have 3 test cases (with custom planTrigger, default and without one) but the only place the intent of the test is visible is through Instance names. Which is better than nothing, but I bet in a few months this won't be so obvious to the reader.

[alenkacz]

@justinbarrick I was thinking about it over weekend. I was leaning towards having a 'root' yaml per test case, where you would define steps and maybe some comments. Then if people write that in a meaningful way it would be easier to get a high-level overview of what is that particular test case doing

[jbarrick-mesosphere]

@alenkacz Agreed! I'll spec something out on the KEP.

[gkleiman]

@alenkacz , @jbarrick-mesosphere: in the meantime I added some comments to the first step of each test. Let me know whether you like them =).

[gkleiman]

@jbarrick-mesosphere I split the upgrade and param change tests out into smaller tests. Please take another look =).

I think it would be nice if we could nest all the upgrade/param-change tests within parent directories to make it obvious that they're related.

[oliviabarrick]

I like the nesting idea a lot, I’ll have to spec that one out too.