KEYCLOAK-13632 E2E tests refactoring

[mhajas]

JIRA ID

Additional Information

I completely forgot to base this work on #164. I will do the rebase once it is merged.

This PR is just a proposal on how e2e tests could look like, I am completely open to any discussion/suggestions.

Verification Steps

Checklist:

• Verified by team member
• Comments where necessary
• Automated Tests
• Documentation changes if necessary

Additional Notes

[coveralls]

Coverage increased (+0.07%) to 41.338% when pulling 26a44dc on mhajas:KEYCLOAK-13632-Extend-testing into df09dab on keycloak:master.

[slaskawi]

I think this is a nice step in good direction. However, I believe we need some changes...

At first, the our Operator is a "Namespaced Operator". This means it observes only one namespace and reacts on changes in it (however, we need only a few enhancements and we'll be "Global or Multinamespace Operator"). I just realized, this means running multiple tests in parallel in different namespaces won't work. All tests need to be executed in a single namespace.

I would like the testsuite to be designed with parallel execution in mind. This means we at least need 2 types of tests in the hierarchy - tests that operate on deployed Keycloak (those can be parallel) and tests that modify Keycloak installation (like spinning up a new cluster, migrations, etc). Here is a nice example, how Go enables you to run things in a sequence/parallel.

I'm +1 in aligning tests with specific CRs (I prefer to refer to those objects CRs as they are Custom Resources, the CRD is a Custom Resource Definition file, so essentially, it's the YAML file we have in the deploy/crds directory).

Finally, one technical note - please use structs and try to pass objects as much as you can by value (not with a pointer). When it comes to executing things in parallel, this will save us a ton of time (especially when it comes to investigating bugs). With structs you can easily reach their internal content without accessors/mutators.

[mhajas]

I think this is a nice step in good direction. However, I believe we need some changes...

At first, the our Operator is a "Namespaced Operator". This means it observes only one namespace and reacts on changes in it (however, we need only a few enhancements and we'll be "Global or Multinamespace Operator"). I just realized, this means running multiple tests in parallel in different namespaces won't work. All tests need to be executed in a single namespace.

I would like the testsuite to be designed with parallel execution in mind. This means we at least need 2 types of tests in the hierarchy - tests that operate on deployed Keycloak (those can be parallel) and tests that modify Keycloak installation (like spinning up a new cluster, migrations, etc). Here is a nice example, how Go enables you to run things in a sequence/parallel.

What do you mean by modifying Keycloak installation? Do you mean changing CR, or rerunning the operator with different variables? At the moment, I can't imagine how we would add any test which is spinning up a new cluster.

I can imagine, that we would add the creating of new namespace and then using it for creating CRs. Actually, it shouldn't be that hard, because we already send the namespace value for CR creating. So having multi namespace operator running tests in parallel should be working with this little change.

I'm +1 in aligning tests with specific CRs (I prefer to refer to those objects CRs as they are Custom Resources, the CRD is a Custom Resource Definition file, so essentially, it's the YAML file we have in the deploy/crds directory).

I used CRD because I thought we will have one test file per CRD and then in tests we can create/update/delete CRs as we like. But if you want, I can change it to CR and then create a file per CR.

Finally, one technical note - please use structs and try to pass objects as much as you can by value (not with a pointer). When it comes to executing things in parallel, this will save us a ton of time (especially when it comes to investigating bugs). With structs you can easily reach their internal content without accessors/mutators.

Sorry, I am maybe a little bit affected by C++ I did some time ago. Do you have some specific place on your mind?

[slaskawi]

What do you mean by modifying Keycloak installation? Do you mean changing CR, or rerunning the operator with different variables? At the moment, I can't imagine how we would add any test which is spinning up a new cluster.

@mhajas No, I mean spinning up Keycloak installation or testing migrations. Something that takes Keycloak down and spins it up again. Tests for other CRs could probably assume that Keycloak is already running (and ready).

I can imagine, that we would add the creating of new namespace and then using it for creating CRs. Actually, it shouldn't be that hard, because we already send the namespace value for CR creating. So having multi namespace operator running tests in parallel should be working with this little change.

@mhajas As I mentioned before, currently Keycloak Operator monitors a single namespace. Multi-namespace support requires implementing KEYCLOAK-12023.

I used CRD because I thought we will have one test file per CRD and then in tests we can create/update/delete CRs as we like. But if you want, I can change it to CR and then create a file per CR.

@mhajas That's fine. We can stick to this convention.

Sorry, I am maybe a little bit affected by C++ I did some time ago. Do you have some specific place on your mind?

@mhajas I think I had getKeycloakCR in mind. We could refactor this to be a struct (and actually a constant too).

So what is the status of this PR? Ready to be reviewed? Or you're still working on it?

[mhajas]

I will change this to a variable once #165 is merged. Importing common package at the moment is causing issues with parameter parsing I mentioned you on the call.

[mhajas]

@slaskawi I think this is ready to review. There is just one thing, I will fix it later if this PR is merged before 165.

[slaskawi]

The code LGTM. However, Travis is sad: https://travis-ci.org/github/keycloak/keycloak-operator/pull_requests

[mhajas]

@slaskawi I made Travis change his mind: https://travis-ci.org/github/keycloak/keycloak-operator/builds/681523106