Copilot commented on code in PR #2501:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2501#discussion_r2262433691
##########
test/e2e/ingress/ingress.go:
##########
@@ -231,81 +232,93 @@ spec:
Status(200)
})
- It("Proxy External Service", func() {
+ PIt("Proxy External Service", func() {
By("create GatewayProxy")
- gatewayProxy := fmt.Sprintf(gatewayProxyYaml,
s.Deployer.GetAdminEndpoint(), s.AdminKey())
- err :=
s.CreateResourceFromStringWithNamespace(gatewayProxy, "default")
+ gatewayProxy := fmt.Sprintf(gatewayProxyYaml,
s.Namespace(), s.Deployer.GetAdminEndpoint(), s.AdminKey())
+ err :=
s.CreateResourceFromStringWithNamespace(gatewayProxy, s.Namespace())
Expect(err).NotTo(HaveOccurred(), "creating
GatewayProxy")
time.Sleep(5 * time.Second)
By("create Default IngressClass")
- err =
s.CreateResourceFromStringWithNamespace(defaultIngressClass, "")
+ err =
s.CreateResourceFromStringWithNamespace(fmt.Sprintf(defaultIngressClass,
s.GetControllerName(), s.Namespace()), s.Namespace())
Expect(err).NotTo(HaveOccurred(), "creating Default
IngressClass")
time.Sleep(5 * time.Second)
By("create Ingress")
- err =
s.CreateResourceFromString(ingressWithExternalName)
+ ingressName := s.Namespace() + "-external"
+ err =
s.CreateResourceFromStringWithNamespace(fmt.Sprintf(ingressWithExternalName,
ingressName), s.Namespace())
Expect(err).NotTo(HaveOccurred(), "creating Ingress
without IngressClass")
- time.Sleep(5 * time.Second)
By("checking the external service response")
- s.NewAPISIXClient().
- GET("/get").
- WithHost("httpbin.external").
- Expect().
- Status(http.StatusOK)
+ time.Sleep(10 * time.Minute)
+ s.RequestAssert(&scaffold.RequestAssert{
+ Method: "GET",
+ Path: "/get",
+ Host: "httpbin.external",
+ Check:
scaffold.WithExpectedStatus(http.StatusOK),
+ Interval: time.Second * 10,
+ Timeout: 3 * time.Minute,
+ })
Review Comment:
The sleep duration of 10 minutes is excessively long and will significantly
slow down test execution. Consider reducing this to a more reasonable duration
like 30 seconds or use Eventually() blocks instead.
```suggestion
Eventually(func(g Gomega) {
s.RequestAssert(&scaffold.RequestAssert{
Method: "GET",
Path: "/get",
Host: "httpbin.external",
Check:
scaffold.WithExpectedStatus(http.StatusOK),
Interval: time.Second * 10,
Timeout: 3 * time.Minute,
})
}, 3*time.Minute, 10*time.Second).Should(Succeed())
```
##########
test/e2e/ingress/ingress.go:
##########
@@ -231,81 +232,93 @@ spec:
Status(200)
})
- It("Proxy External Service", func() {
+ PIt("Proxy External Service", func() {
By("create GatewayProxy")
- gatewayProxy := fmt.Sprintf(gatewayProxyYaml,
s.Deployer.GetAdminEndpoint(), s.AdminKey())
- err :=
s.CreateResourceFromStringWithNamespace(gatewayProxy, "default")
+ gatewayProxy := fmt.Sprintf(gatewayProxyYaml,
s.Namespace(), s.Deployer.GetAdminEndpoint(), s.AdminKey())
+ err :=
s.CreateResourceFromStringWithNamespace(gatewayProxy, s.Namespace())
Expect(err).NotTo(HaveOccurred(), "creating
GatewayProxy")
time.Sleep(5 * time.Second)
By("create Default IngressClass")
- err =
s.CreateResourceFromStringWithNamespace(defaultIngressClass, "")
+ err =
s.CreateResourceFromStringWithNamespace(fmt.Sprintf(defaultIngressClass,
s.GetControllerName(), s.Namespace()), s.Namespace())
Expect(err).NotTo(HaveOccurred(), "creating Default
IngressClass")
time.Sleep(5 * time.Second)
By("create Ingress")
- err =
s.CreateResourceFromString(ingressWithExternalName)
+ ingressName := s.Namespace() + "-external"
+ err =
s.CreateResourceFromStringWithNamespace(fmt.Sprintf(ingressWithExternalName,
ingressName), s.Namespace())
Expect(err).NotTo(HaveOccurred(), "creating Ingress
without IngressClass")
- time.Sleep(5 * time.Second)
By("checking the external service response")
- s.NewAPISIXClient().
- GET("/get").
- WithHost("httpbin.external").
- Expect().
- Status(http.StatusOK)
+ time.Sleep(10 * time.Minute)
+ s.RequestAssert(&scaffold.RequestAssert{
+ Method: "GET",
+ Path: "/get",
+ Host: "httpbin.external",
+ Check:
scaffold.WithExpectedStatus(http.StatusOK),
+ Interval: time.Second * 10,
+ Timeout: 3 * time.Minute,
+ })
})
- It("Delete Ingress during restart", func() {
+ PIt("Delete Ingress during restart", func() {
By("create GatewayProxy")
- gatewayProxy := fmt.Sprintf(gatewayProxyYaml,
s.Deployer.GetAdminEndpoint(), s.AdminKey())
- err :=
s.CreateResourceFromStringWithNamespace(gatewayProxy, "default")
+ gatewayProxy := fmt.Sprintf(gatewayProxyYaml,
s.Namespace(), s.Deployer.GetAdminEndpoint(), s.AdminKey())
+ err :=
s.CreateResourceFromStringWithNamespace(gatewayProxy, s.Namespace())
Expect(err).NotTo(HaveOccurred(), "creating
GatewayProxy")
time.Sleep(5 * time.Second)
By("create Default IngressClass")
- err =
s.CreateResourceFromStringWithNamespace(defaultIngressClass, "")
+ err =
s.CreateResourceFromStringWithNamespace(fmt.Sprintf(defaultIngressClass,
s.GetControllerName(), s.Namespace()), s.Namespace())
Expect(err).NotTo(HaveOccurred(), "creating Default
IngressClass")
time.Sleep(5 * time.Second)
By("create Ingress with ExternalName")
- err =
s.CreateResourceFromString(ingressWithExternalName)
+ ingressName := s.Namespace() + "-external"
+ err =
s.CreateResourceFromStringWithNamespace(fmt.Sprintf(ingressWithExternalName,
ingressName), s.Namespace())
Expect(err).NotTo(HaveOccurred(), "creating Ingress
without IngressClass")
time.Sleep(5 * time.Second)
By("create Ingress")
- err = s.CreateResourceFromString(defaultIngress)
+ err =
s.CreateResourceFromStringWithNamespace(fmt.Sprintf(defaultIngress,
s.Namespace()), s.Namespace())
Expect(err).NotTo(HaveOccurred(), "creating Ingress
without IngressClass")
- time.Sleep(5 * time.Second)
By("checking the external service response")
- s.NewAPISIXClient().
- GET("/get").
- WithHost("httpbin.external").
- Expect().
- Status(http.StatusOK)
-
- s.NewAPISIXClient().
- GET("/get").
- WithHost("default.example.com").
- Expect().
- Status(200)
-
+ time.Sleep(10 * time.Minute)
Review Comment:
Another 10-minute sleep that will make tests unnecessarily slow. This should
be reduced to a reasonable duration or replaced with proper wait conditions.
```suggestion
// Removed unnecessary 10-minute sleep; rely on
RequestAssert for readiness
```
##########
test/e2e/gatewayapi/httproute.go:
##########
@@ -1944,39 +2062,48 @@ spec:
`
It("Should sync ApisixRoute during startup", func() {
By("apply ApisixRoute")
-
Expect(s.CreateResourceFromString(route2)).ShouldNot(HaveOccurred(), "applying
HTTPRoute with non-existent parent")
- s.ResourceApplied("HTTPRoute", "httpbin", route, 1)
+ Expect(s.CreateResourceFromStringWithNamespace(route2,
s.Namespace())).ShouldNot(HaveOccurred(), "applying HTTPRoute with non-existent
parent")
+ s.ResourceApplied("HTTPRoute", "httpbin",
fmt.Sprintf(route, s.Namespace()), 1)
s.RequestAssert(&scaffold.RequestAssert{
- Method: "GET",
- Path: "/get",
- Host: "httpbin",
- Check:
scaffold.WithExpectedStatus(http.StatusOK),
+ Method: "GET",
+ Path: "/get",
+ Host: "httpbin",
+ Check:
scaffold.WithExpectedStatus(http.StatusOK),
+ Interval: time.Second * 2,
+ Timeout: 30 * time.Second,
})
s.RequestAssert(&scaffold.RequestAssert{
- Method: "GET",
- Path: "/get",
- Host: "httpbin2",
- Check:
scaffold.WithExpectedStatus(http.StatusNotFound),
+ Method: "GET",
+ Path: "/get",
+ Host: "httpbin2",
+ Check:
scaffold.WithExpectedStatus(http.StatusNotFound),
+ Interval: time.Second * 2,
+ Timeout: 30 * time.Second,
})
+ time.Sleep((8 * time.Second))
Review Comment:
Unnecessary parentheses around the sleep duration expression. This should be
simplified to `time.Sleep(8 * time.Second)`.
```suggestion
time.Sleep(8 * time.Second)
```
##########
test/e2e/scaffold/apisix_deployer.go:
##########
@@ -227,11 +230,12 @@ func (s *APISIXDeployer) deployDataplane(opts
*APISIXDeployOptions) *corev1.Serv
Expect(err).ToNot(HaveOccurred(), "executing template")
k8s.KubectlApplyFromString(s.GinkgoT, kubectlOpts, buf.String())
-
- err = framework.WaitPodsAvailable(s.GinkgoT, kubectlOpts,
metav1.ListOptions{
- LabelSelector: "app.kubernetes.io/name=apisix",
- })
- Expect(err).ToNot(HaveOccurred(), "waiting for gateway pod ready")
+ if opts.Replicas == nil || (opts.Replicas != nil && *opts.Replicas > 0)
{
Review Comment:
The condition can be simplified. Since `opts.Replicas != nil` is redundant
when already checked with `||`, this can be written as `if opts.Replicas == nil
|| *opts.Replicas > 0 {`.
```suggestion
if opts.Replicas == nil || *opts.Replicas > 0 {
```
##########
test/e2e/crds/v2/route.go:
##########
@@ -739,13 +785,15 @@ spec:
weight: 0
`
By("apply ApisixRoute with zero-weight backend")
- applier.MustApplyAPIv2(types.NamespacedName{Namespace:
s.Namespace(), Name: "default"}, new(apiv2.ApisixRoute),
- fmt.Sprintf(apisixRouteSpec,
s.Deployer.GetAdminServiceName()))
+ applier.MustApplyAPIv2(types.NamespacedName{Namespace:
s.Namespace(), Name: "default"},
+ new(apiv2.ApisixRoute),
+ fmt.Sprintf(apisixRouteSpec, s.Namespace(),
s.Namespace(), s.Deployer.GetAdminServiceName()))
verifyRequest := func() int {
return
s.NewAPISIXClient().GET("/get").WithHost("httpbin.org").Expect().Raw().StatusCode
}
By("wait for route to be ready")
+
Review Comment:
There's an empty line followed by a comment without any actual waiting
logic. This appears to be incomplete - either add the waiting logic or remove
the misleading comment.
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@apisix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
