Re: [PR] [fix][test] Make admin and transaction tests tolerant of broker behavior changes [pulsar-client-go]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #1545:
URL: https://github.com/apache/pulsar-client-go/pull/1545#discussion_r4281048742


##########
pulsar/helper_for_test.go:
##########
@@ -157,13 +160,65 @@ func topicStats(topic string) (map[string]interface{}, error) {
 	return metadata, err
 }
 
+// transactionStats queries the transaction metadata admin endpoint. Since
+// apache/pulsar#26773 that endpoint requires super-user access, so the request must be
+// authenticated. The plain-text httpGet helper sends no credentials and would be rejected
+// as the "anonymous" role (HTTP 401), so use the TLS web service with the test client
+// certificate instead, whose CN ("admin") is a super-user role in standalone.conf.
 func transactionStats(id *TxnID) (map[string]interface{}, error) {
 	var metadata map[string]interface{}
 	path := fmt.Sprintf("admin/v3/transactions/transactionMetadata/%d/%d", id.MostSigBits, id.LeastSigBits)
-	err := httpGet(path, &metadata)
+	err := httpGetWithTLS(path, &metadata)
 	return metadata, err
 }
 
+// httpGetWithTLS performs an HTTPS GET against the broker's web service, authenticating
+// with the test client certificate. It is used for admin endpoints that require a
+// super-user role.
+func httpGetWithTLS(requestPath string, out interface{}) error {
+	cert, err := tls.LoadX509KeyPair(tlsClientCertPath, tlsClientKeyPath)
+	if err != nil {
+		return err
+	}
+	caCert, err := os.ReadFile(caCertsPath)
+	if err != nil {
+		return err
+	}
+	pool := x509.NewCertPool()
+	if !pool.AppendCertsFromPEM(caCert) {
+		return fmt.Errorf("failed to parse CA certificates from %s", caCertsPath)
+	}
+	client := &http.Client{
+		Transport: &http.Transport{
+			TLSClientConfig: &tls.Config{
+				Certificates: []tls.Certificate{cert},
+				RootCAs:      pool,
+			},
+		},
+	}

Review Comment:
   The new HTTPS test client has no timeout, so a stalled broker/network path can hang tests indefinitely. Set a reasonable `http.Client.Timeout` (and optionally transport-level timeouts) to bound test runtime and prevent CI deadlocks.



##########
pulsaradmin/pkg/admin/namespace_test.go:
##########
@@ -237,23 +241,23 @@ func TestSetTopicAutoCreation(t *testing.T) {
 				Allow: true,
 				Type:  utils.NonPartitioned,
 			},
-			errReason: "Tenant does not exist",
+			errReasons: []string{"Tenant does not exist", "Namespace does not exist"},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			namespace, _ := utils.GetNamespaceName(tt.namespace)
 			err := admin.Namespaces().SetTopicAutoCreation(*namespace, tt.config)
-			if tt.errReason == "" {
-				assert.Equal(t, nil, err)
+			if len(tt.errReasons) == 0 {
+				assert.NoError(t, err)
 
 				err = admin.Namespaces().RemoveTopicAutoCreation(*namespace)
-				assert.Equal(t, nil, err)
-			}
-			if err != nil {
-				restError := err.(rest.Error)
-				assert.Equal(t, tt.errReason, restError.Reason)
+				assert.NoError(t, err)
+				return
 			}
+			require.Error(t, err)
+			restError := err.(rest.Error)
+			assert.Contains(t, tt.errReasons, restError.Reason)

Review Comment:
   This direct type assertion (`err.(rest.Error)`) will panic if `SetTopicAutoCreation` returns a non-`rest.Error` (e.g., transport errors, timeouts, TLS errors). Use `errors.As` (or a checked assertion) to safely extract the `rest.Error`, and fail the test with a clear message if the error isn’t of that type.



-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org