strimzi-kafka-operator icon indicating copy to clipboard operation
strimzi-kafka-operator copied to clipboard

fix: Update TopicOperatorTest to handle async flow correctly

Open katheris opened this issue 1 year ago • 2 comments

Type of change

Select the type of your PR

  • Bugfix

Description

Update the TopicOperatorTest to fix problems with asynchronous tests being completed early before all assertions are run. This fixes issue #7072.

Main changes are:

  • Update the resourceAdded and similar functions so they return a future, rather than creating and flagging a checkpoint
  • Change checkpoints to context.completeNow() where possible
  • Remove the use of CountdownLatch in favour of vertx future composition

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • [ ] Write tests
  • [ ] Make sure all tests pass
  • [ ] Update documentation
  • [ ] Check RBAC rights for Kubernetes / OpenShift roles
  • [ ] Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • [ ] Reference relevant issue(s) and close them after merging
  • [ ] Update CHANGELOG.md
  • [ ] Supply screenshots for visual changes, such as Grafana dashboards

katheris avatar Sep 15 '22 17:09 katheris

Can one of the admins verify this patch?

strimzi-ci avatar Sep 15 '22 17:09 strimzi-ci

This change is complete, but as you can see three of the tests fail. These are:

  • testReconcile_withResource_withKafka_noPrivate_matching
  • testReconcileMetricsWithPausedTopic
  • testReconcileMetricsDeletedTopic

The first one the error is that the expected metrics for the topic cannot be found. These assertions weren't being run previously due to the checkpoint being flagged before the end of the test.

The second two are both failing within the addResource or deleteResource functions and are complaining that the topic isn't in the expected state. Previously these functions ran in parallel with the reconcile call so it is unclear what order they are supposed to be in to pass correctly. If I comment out the specific assertions that are failing, the others assertions in those tests do pass.

katheris avatar Sep 16 '22 12:09 katheris

@katheris I assume you want to wait for @SamBarker before we merge this?

scholzj avatar Sep 26 '22 14:09 scholzj

@scholzj yeah, let's wait for him to look over the latest changes

katheris avatar Sep 26 '22 14:09 katheris