apache / apisix-ingress-controller

APISIX Ingress Controller for Kubernetes
https://apisix.apache.org/
Apache License 2.0
972 stars 337 forks source link

e2e: introduce phase function to split cases #1226

Open lingsamuel opened 1 year ago

lingsamuel commented 1 year ago

Currently our e2e case has a lot of duplicate code snippets, which will be harder to maintain as time goes on.

A Phase function is a simple func() type function. Phase functions can be shared between different e2e cases.

For a typical case, we may have a "data preparation" phase, and a "validation" phase to verify that our controller is working properly.

However, during the data preparation phase, we may need to support several different CRDs, such as ApisixRoute and Ingress. We simply copy and paste the entire case now. If we had the concept of phase, we could split the data preparation into a shared data preparation function and a specific CRD preparation function. In this way, we can share other code snippets without any copy and paste.

This can reduce a lot of duplicate code and also greatly improve code readability.

tao12345666333 commented 1 year ago

SGTM

AlinsRan commented 1 year ago

+1

stillfox-lee commented 1 year ago

If we had the concept of phase, we could split the data preparation into a shared data preparation function and a specific CRD preparation function.

From my point of view, CRD preparation function can generate all kinds of Resource's yaml. But, what's shared data preparation function? Can you show me the specific test case?

lingsamuel commented 1 year ago

The general idea is that our e2e cases are a combination of many small cases. We can achieve complex cases by combining small cases. Here are some possible code snippets that describe how I think this proposal would work.

The API design is very early and unproven, any suggestions are welcome!

Let's assume that we have a type called Tester that will contain the automation features we need.

  1. Code snippet of basic functionality
// Assuming we are testing the ApisixRoute match rule
tester := CreateRouteTester(&RouteTesterOption{header: "X-Foo", headerMatch: "bar", url: "/ip"})
// data preparation
tester.ApplyRoute() // ApisixRoute will be generated automatically base on the route rule and other options will remain default.
// validation
tester.ExpectAccess() // make a request to the route base on our route rule and expect it to succeed.
tester.ExpectAccessFailedWithWrongRule() // make a request to the route but with a wrong options and expect it to failed

Here we can notice that both ExpectAccess and ExpectAccesFailedWithWrongRule should always be called together in our validation. So we provide another function ExecuteAllValidators() to do this.

  1. Code snippet of update functionality
// data preparation
tester.ApplyWrongRoute() // Create a wrong ApisixRoute that doesn't match our route rule
// validation
tester.ExpectAccessFailed() // make a request to the route with correct route rule, and expect it fail
// data update
tester.ApplyRoute() // Apply the correct ApisixRoute, it's a update behavior here
// validation
tester.ExecuteAllValidators() // It should be OK now
  1. A case of tester combination

Some other cases may also need to use ApisixRoute. They can reuse the tester now. For example, we need to verify that namespace selector is working correctly.

ns := "test-namespace"
namespaceTester := CreateNamespaceTester(&NamespaceTesterOption{name: ns})
routeTester := CreateRouteTester(&RouteTesterOption{namespace: ns})
// data preparation
namespaceTester.CreateWithWatchLabel()
routeTester.ApplyRoute()
// validation
routeTester.ExecuteAllValidators()
// data preparation
namespaceTester.CreateWithoutWatchLabel()
routeTester.ApplyRoute()
// validation
routeTester.ExpectAccessFailed()
// update to watch
namespaceTester.AddWatchLabel()
// validate it works
routeTester.ExecuteAllValidators()
// update to unwatch
namespaceTester.RemoveWatchLabel()
// validate it doesn't work
routeTester.ExpectAccessFailed()

We have only tested that the request is working as expected so far. What if we want to make sure the route resource is actually removed in APISIX? Suppose we have a ExpectNoRouteInAPISIX function to do this.

In this case, we notice that the ExpectedAccessFailed function is always called together with ExpectNoRouteInAPISIX. Here is my solution.

tester := CreateTester() // a generic test runner
tester.RegisterPhase("data-preparation", function() {
  namespaceTester.CreateWithoutWatchLabel()
  routeTester.ApplyRoute()
})
tester.RegisterPhase("watch", namespaceTester.AddWatchLabel)
tester.RegisterPhase("unwatch", namespaceTester.RemoveWatchLabel)

tester.RegisterValidatorForPhases([]string{"data-preparation", "unwatch"}, function() {
  routeTester.ExpectAccessFailed()
  routeTester.ExpectNoRouteInAPISIX()
})
tester.RegisterValidatorForPhases([]string{"watch"}, function() {
  routeTester.ExecuteAllValidators()
})

tester.RunPhases("data-preparation", "watch", "unwatch") //  all done!