michaeljmarshall commented on a change in pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#discussion_r806063357
##########
File path:
tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/topicpolicies/SchemaCompatibilityStrategyTest.java
##########
@@ -19,67 +19,68 @@
package org.apache.pulsar.tests.integration.cli.topicpolicies;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
import org.apache.pulsar.common.policies.data.SchemaCompatibilityStrategy;
import org.apache.pulsar.tests.integration.docker.ContainerExecResult;
-import org.apache.pulsar.tests.integration.suites.PulsarTestSuite;
+import org.apache.pulsar.tests.integration.suites.PulsarCliTestSuite;
import org.awaitility.Awaitility;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-public class SchemaCompatibilityStrategyTest extends PulsarTestSuite {
+public class SchemaCompatibilityStrategyTest extends PulsarCliTestSuite {
+ @BeforeMethod(alwaysRun = true)
Review comment:
Is this method supposed to be annotated with `@BeforeClass`?
`super.before()` is annotated that way. Given that `enableTopicPolicies()` sets
broker config and brokers are only set up once per class, I think we want
`@BeforeClass`.
```suggestion
@BeforeClass(alwaysRun = true)
```
##########
File path:
tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/topicpolicies/SchemaCompatibilityStrategyTest.java
##########
@@ -19,67 +19,68 @@
package org.apache.pulsar.tests.integration.cli.topicpolicies;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
import org.apache.pulsar.common.policies.data.SchemaCompatibilityStrategy;
import org.apache.pulsar.tests.integration.docker.ContainerExecResult;
-import org.apache.pulsar.tests.integration.suites.PulsarTestSuite;
+import org.apache.pulsar.tests.integration.suites.PulsarCliTestSuite;
import org.awaitility.Awaitility;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-public class SchemaCompatibilityStrategyTest extends PulsarTestSuite {
+public class SchemaCompatibilityStrategyTest extends PulsarCliTestSuite {
+ @BeforeMethod(alwaysRun = true)
Review comment:
Okay, I can see how state of individual tests can impact each other.
However, the current design of the `PulsarCliTestSuite` does not align with its
usage here. Is there a way to ensure test isolation without resetting the
cluster? If not, I think it'd be better to make it clearer that the
`PulsarCliTestSuite#before` method should be called before each test and that
the abstract suite is designed to make individual tests isolated.
A secondary concern is that it has to be somewhat expensive to reset
brokers. We should try to limit the cost of basic tests, if possible.
##########
File path:
tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/topicpolicies/SchemaCompatibilityStrategyTest.java
##########
@@ -19,67 +19,68 @@
package org.apache.pulsar.tests.integration.cli.topicpolicies;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
import org.apache.pulsar.common.policies.data.SchemaCompatibilityStrategy;
import org.apache.pulsar.tests.integration.docker.ContainerExecResult;
-import org.apache.pulsar.tests.integration.suites.PulsarTestSuite;
+import org.apache.pulsar.tests.integration.suites.PulsarCliTestSuite;
import org.awaitility.Awaitility;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-public class SchemaCompatibilityStrategyTest extends PulsarTestSuite {
+public class SchemaCompatibilityStrategyTest extends PulsarCliTestSuite {
+ @BeforeMethod(alwaysRun = true)
Review comment:
Sure, let's create an issue.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
