[WIP] CLOUDP-178750: Add deletion protection to deployments

[josvazg]

All Submissions:

• Have you signed our CLA?
• Put closes #XXXX in your comment to auto-close the issue that your PR fixes (if there is one).
• Update docs/release-notes/release-notes.md if your changes should be included in the release notes for the next release.

[github-actions]

https://app.codecov.io/github/mongodb/mongodb-atlas-kubernetes/commit/0157cb3fbea4168a6c49f33268d4df6f6236e119

[helderjs]

Because you mentioned in the standup today, I jumped here for a quick look and I have some thoughts on this.

• Verbosity - It's too much extra code we need to write in order to start really testing. This will make writing tests boring and I don't think make sense to us spend a lot of time writing response for a sequence of API calls manually when the API is easily available to us even at the cost of a higher execution time.

• Maintainability - This tends to grow and become hard to maintain once it's unstructured code (not implementing any interface we could rely on).

• Traceability - I have seen (and worked) on this kind of manual mock before and when an assertion fails, because of a typo, for example, we will not get the mismatch from the diff and we would have to search in the middle of a bunch of JSON string where the typo is.

As last, while we are consumers of the Atlas API our project consumes it through abstraction, the atlas-go-client, I believe we should be mocking this abstraction and not going a level downstairs and mimicking the API. As the atlas-go-client is a third-party dependency, I think we should take the direction of wrapping it by defining interfaces and composing them to have the needed behavior and then we can mock these interfaces in an automated way (gomock?).

Feel free to bring other guys to the discussion or take it to a call for a quicker feedback

[josvazg]

Thanks for the feedback @helderjs on this exploratory work.

I am too not happy about the amount of work the mocking required, I was more time fixing the mock than the actual test or code. But to be fair, this amount of work is coming from two different places:

1. A one off investment on testing infrastructure to support testing our logic against predefined Atlas replies. This is an effort that will recede with time. Note this capability not only allows us to work faster, but it also allows us to run more tests in less time in the CI AND write tests that would not do easily if the initial state in Atlas is not easy to get, like in a weird failure case we want to protect the code against regressions.
2. The per test mocking effort of setting the fake atlas to the desired initial state and subsequent behaviour. This is not going to recede in the same way, but there will be common patterns we won't need to reinvent for each new test, like the project initialisation part, which BTW is also the biggest mock in this case.

Maintainability: I was already working on a refactor of the common bits so that we could define the mocks as data, an ordered sequence of method+path expected and their associated reply closure. I will probably show that today even if we revert later, just to see the separation between test infra an per test mocking.

_ Traceability_ I fully agree, testing with typed code and data is preferable to handcrafted strings... unless those strings were not handcrafted! more on that later...

As last, while we are consumers of the Atlas API our project consumes it through abstraction, the atlas-go-client, I believe we should be mocking this abstraction and not going a level downstairs and mimicking the API.

That might be a better approach. Do you know of any prior work we could leverage there? Or maybe support from the library itself?

One possible improvement would be a test recorder. We run the test against a real atlas, but our code is instrumented to record all interactions. The interactions are recorded to a file, we prune any duplicates or repetitions we do not need and we have our mock ready to go. There is no much room for typos on recorded real protocol interactions. We can do that at the protocol level more easily maybe, but we could try to do the same recording at the library boundary.

But the one time investment on test infra is not avoidable IMHO. And I think it is needed.

For instance, this tests includes 6 test cases that would require to deploy an Atlas cluster to start each test. I do not have an idea how long that would take if I remove the mock and test for real. And also would need to account for atlas deployment cleanup, which was not needed with the mock.

[helderjs]

My idea so far is based on the AtlasCLI approach, I basically described what they are doing. They have stores which are interfaces that are implemented wrapping the Atlas client.
Then they use gomock to generate the mocks and use them on the tests.
We agree that some groundwork must be done before in order to have all the bits to use this strategy, and it's quite big work. I'm just targeting a long-term solution where the balance is positive (this one has downsides too, off course)
I think we can push product a little bit a prioritize this as a separate task, as IMHO, it would be very complicated to introduce it in ongoing/upcoming tasks

[josvazg]

Ok, I will look how that works im more detail.

In the meantime I made an update with the refactored protocol level mocking.

Next, I will create a new PR without the protocol mocking and close this one pointing to it.

BTW I am worried the non mocked versions of the current tests might not be practical, they might take way too much time and be way too flaky to be useful. I might have to rethink the tests completely and leave only a basic protection test at integration level.

[josvazg]

Closing in favor of #1036
Basically the same thing without any protocol test mocking. And some changes to adjust for that change, like removing the deployment when not done by the test and tweaking the timeouts and polling intervals.