Testing about eksctl

[richardcase]

@Raffo @errordeveloper - what about the following initially:

• Add tests to each of the packages (like we've done with the kubeconfig change). The decision here is whether the tests for a package are contained within the package and have access to all the non-exported functions/structs/etc or if instead the tests site in a test package and test only via the exported functions/structs/etc (seeing only what the user of the package would see). I used the later for the kubeconfig tests.
• We can mock the AWS services using the interfaces and inject these mocks where required for the unit tests (this may require some refactoring).
• We create a separate integration test suite that tests against AWS (when we've worked out who can pay/sponsor). I quite like how minikube have done this. We'd have to perform operations with eksctl and then check its done what we expected using the AWS sdk directly (or something else).
• Start with the get command
• Add code coverage output

So just standard testing.

[errordeveloper]

We might want to start on this before 0.1.0, but I wouldn't block 0.1.0 on lack of automated testing, unless others strongly object.

[Raffo]

I was having a look at the code today and I totally recommend having having a proper testing strategy in place before implementing other major features. I guess 0.1.0 can be released, but I would give priority to this before any other major changes.
There is a bunch that can be done even with the AWS SDK and even if you are using CloudFormation, we can test things like, for example, the use of the returned values (by mocking of course) and the other part of the code that call CF (i.e. the code that waits on the stack). That would help at least to figure in a better way if the code is well structured in terms of how the function are divided. IMO it is simple enough till now and that would be a pretty good investment if done right now.

I agree in also putting some focus on integration testing, if someone is sponsoring an AWS account for such tests it wouldn't be so hard to setup.

Please note that is only my opinion 😄

[richardcase]

@errordeveloper - happy to look at this if you want?

[Raffo]

Happy to help with the review @richardcase 😉

[errordeveloper]

Hey folks, please go ahead! Would be good to outline a short plan and discuss it before anyone spends a lot of time on this.

…

On Fri, 22 Jun 2018, 12:03 am Raffaele Di Fazio, ***@***.***> wrote: Happy to help with the review @richardcase <https://github.com/richardcase> 😉 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#46 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPWSyXrpIAKNStCjqWnWvJLBG8Kp_4-ks5t_JaygaJpZM4UccK7> .

[Raffo]

Sounds good to me.

@richardcase what's your plan for that?

[richardcase]

@Raffo - i will have a look and come up with a plan.

There are things like localstack and also the Go SDK has interfaces for the services to help build mocks, like this for CloudFormation (i haven't checked interfaces for the other dependent services like STS).

[Raffo]

I think the approach with interfaces would be good and that's what I've used in the past. We've used a similar approach while implementing testing for external DNS here: https://github.com/kubernetes-incubator/external-dns/blob/master/provider/aws.go#L79

[Raffo]

I really like the plan 👍

I'd start with the basic unit testing inside the individual packages as this can guide some early refactoring of the code. I also totally agree with using the AWS service mocking for that as it should be just what we need for that job.

Absolutely let's have the coverage easy visible for contributors such that we makes sure that future contributors can also provide adeguate tests.

I would give the integration testing to a later step once we have the steps above done, but it's absolutely also very important.

EDIT: Additionally, do you feel okay with getting started with that or do you want me to take over a part of it? WDYT?

[errordeveloper]

I would start by finding out what it takes to mock CloudFormation, as to me it seem kind of complex, but may I just don't know. In my mind integration tests (besides the question of who gets the bill) would buy as more for much less code, testing different code-path maybe be tricky, but starting by simply testing eksctl create cluster seems plausible to me. If you folks are quite comfortable with mocking CloudFormation, I won't oppose at all. Right now, I am just unable to imagine how it would work (seems like it would be a lot of code)...

[richardcase]

I've started to look at this and using mockery to mock the AWS services. I've one test working now, not a lot i know but just to prove that the approach works.

You can see the WIP on the PR.

[Raffo]

Nice that the first tests are now merged. Now that we have a base for the tests in place we can probably go in parallel and add more.

[richardcase]

I'll look at adding a base integration test now.

[errordeveloper]

@richardcase should we make integration tests a requirement for one of the next patch releases?

Manual test would be okay for now, as long as it's well defined (not just "create a cluster and deploy some app, then check it works, run e2e suite if in doubt").

We might want to look into running conformance suite, but I think deploying some workloads would be just fine for now. My colleague @dlespiau wrote a very nice and simple library that should help with this, we are using it in a few projects already.

I think eventually it'd be nice to allow users to easily validate their clusters, so something like

eksctl validate [--workload-test] [--kubernetes-conformance-test] [--full-kubernetes-e2e-test] [--node-problem-detector]

should probably become a thing eventually, especially when we are gonna allow custom node AMIs etc.

[richardcase]

should we make integration tests a requirement for one of the next patch releases?

Yes i think we should. I'll take a look at this next and will look at the package by @dlespiau. Also, what minikube do is quite good.

I really like the idea of validating clusters using eksctl and think this could come after the initial integration tests.

[richardcase]

@errordeveloper - i've started work on the integration tests. Are you happy for us to use this issue or do we want a separate one for integration tests?

[errordeveloper]

Yes, let's create a separate umbrella issue.

…

On Mon, 6 Aug 2018, 3:36 pm Richard Case, ***@***.***> wrote: @errordeveloper <https://github.com/errordeveloper> - i've started work on the integration tests. Are you happy for us to use this issue or do we want a separate one for integration tests? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#46 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPWS26bSNlTaUYK_zplJPfIESi4CIzXks5uOFRggaJpZM4UccK7> .

[richardcase]

@errordeveloper - do you think we can close this now we have #151 for the integration tests?

[errordeveloper]

@richardcase yes, but before we close this, I think it'd make sense to open another set a goal post for coverage - wdyt?

[richardcase]

Sure. And this would be a coverage target for the unit tests only and it will not include the integration tests? If thats the case what do you think to 90% coverage?

[errordeveloper]

I'd guess we start with unit tests for now. Integration coverage is harder to grasp, as you have to think of various flag combos, so I'm not sure how we would make that work in a way that doesn't require creating dozens of clusters...

90% could be a good one to aim for, but I'd also define a lower band (60%-70%?) that's more realistic near-term. So I guess 2 separate issues...

[richardcase]

Closing this issus as we have defined the testing strategy and have started to add tests. We now have new issues to improve various aspects:

• Integration tests #151
• Test coverage to 60% #166
• Test coverage to 90% #167

[Raffo]

We probably don't need to achieve 90% or a specific number, but I believe we should keep track of the coverage anyways as it is a good indication of where the efforts should go.

[richardcase]

I agree. I have removed the 90% target issue and changed the other one to be more generic (#166). We can track coverage with coveralls.