Copilot commented on code in PR #12512:
URL: https://github.com/apache/gluten/pull/12512#discussion_r3588028320
##########
gluten-ut/common/src/test/scala/org/apache/gluten/utils/BackendTestSettings.scala:
##########
@@ -42,18 +46,28 @@ abstract class BackendTestSettings {
suiteSettings
}
+ protected def disableSuite[T: ClassTag](reason: String): Unit = {
+ disableSuite(implicitly[ClassTag[T]].runtimeClass.getCanonicalName, reason)
+ }
+
+ protected def disableSuite(suiteName: String, reason: String): Unit = {
+ require(reason.nonEmpty, "Disable reason must not be empty")
+ if (enabledSuites.containsKey(suiteName)) {
+ throw new IllegalArgumentException("Suite is already enabled: " +
suiteName)
+ }
+ if (disabledSuites.containsKey(suiteName)) {
+ throw new IllegalArgumentException("Duplicated disabled suite: " +
suiteName)
+ }
+ disabledSuites.put(suiteName, reason)
+ }
Review Comment:
`require(reason.nonEmpty, ...)` allows whitespace-only reasons (e.g., " "),
which defeats the purpose of requiring a meaningful disable reason. Trimming
also avoids storing accidental leading/trailing whitespace in `disabledSuites`.
##########
gluten-ut/common/src/test/scala/org/apache/gluten/utils/BackendTestSettings.scala:
##########
@@ -28,12 +28,16 @@ import scala.reflect.ClassTag
abstract class BackendTestSettings {
private val enabledSuites: java.util.Map[String, SuiteSettings] = new
util.HashMap()
+ private val disabledSuites: java.util.Map[String, String] = new
util.HashMap()
protected def enableSuite[T: ClassTag]: SuiteSettings = {
enableSuite(implicitly[ClassTag[T]].runtimeClass.getCanonicalName)
}
protected def enableSuite(suiteName: String): SuiteSettings = {
+ if (disabledSuites.containsKey(suiteName)) {
+ throw new IllegalArgumentException("Suite is already disabled: " +
suiteName)
+ }
if (enabledSuites.containsKey(suiteName)) {
throw new IllegalArgumentException("Duplicated suite name: " + suiteName)
}
Review Comment:
The new `disabledSuites` map stores a disable reason, but `enableSuite`
throws without surfacing that reason. Including it in the exception makes the
new tracking actionable when a suite is accidentally re-enabled.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
