Copilot commented on code in PR #12512:
URL: https://github.com/apache/gluten/pull/12512#discussion_r3584232322
##########
gluten-ut/common/src/test/scala/org/apache/gluten/utils/BackendTestSettings.scala:
##########
@@ -42,8 +46,23 @@ 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)
+ }
Review Comment:
When `disableSuite` is called twice for the same suite, the exception
message drops the original disable reason, which defeats the purpose of keeping
a reason string. Including the existing reason makes debugging
misconfigurations much easier.
##########
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)
+ }
Review Comment:
`disableSuite` requires a non-empty reason, but that reason is currently
never surfaced. In particular, attempting to `enableSuite` for a disabled suite
throws an exception without including the stored reason, which makes it harder
to understand why the suite was disabled.
--
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]
