search

observatorium / thanos-receive-controller

Kubernetes controller to automatically configure Thanos receive hashrings
Apache License 2.0
95 stars 45 forks source link

Improve clarity of tests #31

Closed kakkoyun closed 4 years ago

kakkoyun commented 4 years ago

This PR fixes minor issues that has been introduced by previous PR #30.

cc @brancz

squat commented 4 years ago

PTAL @ https://github.com/observatorium/thanos-receive-controller/pull/30#discussion_r332908597

kakkoyun commented 4 years ago

@squat Regarding the following comment:

If you want to trigger a change, then I think the correct order of operations would be to do:

  1. create klient
  2. bootstrap initial resources
  3. start controller
  4. wait for sync
  5. compare resources

Since we only want to conduct updates on generated configmap when we have changed hashrings, we are making changes on generated and original configmaps after their initial creation. That's why I've put 2 reconciliation loops in those tests, one after creation and one after updates.

  1. create klient
  2. bootstrap initial resources
  3. start controller
  4. wait for sync
  5. update generated and original resources
  6. wait for sync
  7. compare resources

What do you think could we simplify this?

squat commented 4 years ago

OK I see what you are saying now but I think that the multiple re-syncs complicate the logic. The tests should be distilled the most minimal set of operations needed to assert our expectations. The point of this test is to ensure that given an existing generated configmap, the resulting configmap will look how we expect, right? So:

  1. create klient
  2. create initial resources INCLUDING and existing generated configmap (can be more than one func call, does not need to be all in the same func)
  3. run controller
  4. wait for one single sync, since we want to test that after one sync the resources match our expectations
  5. compare