Copilot commented on code in PR #824:
URL: https://github.com/apache/incubator-graphar/pull/824#discussion_r2679427106
##########
maven-projects/spark/graphar/src/test/scala/org/apache/graphar/BaseTestSuite.scala:
##########
@@ -29,10 +29,25 @@ abstract class BaseTestSuite extends AnyFunSuite with
BeforeAndAfterAll {
var spark: SparkSession = _
override def beforeAll(): Unit = {
- if (System.getenv("GAR_TEST_DATA") == null) {
- throw new IllegalArgumentException("GAR_TEST_DATA is not set")
+ def resolveTestData(): String = {
+ Option(System.getenv("GAR_TEST_DATA"))
+ .orElse(Option(System.getProperty("gar.test.data")))
+ .getOrElse {
+ val candidates = Seq("../../testing", "../testing", "testing")
+ candidates
+ .map(p => new java.io.File(p).getAbsoluteFile)
+ .find(d =>
+ new java.io.File(d, "ldbc_sample/csv/ldbc_sample.graph.yml")
+ .exists()
+ )
+ .map(_.getAbsolutePath)
+ .getOrElse(
+ throw new IllegalArgumentException("GAR_TEST_DATA not found")
Review Comment:
The exception type differs from the one used in TestUtil.java, which uses
RuntimeException. Consider using RuntimeException instead of
IllegalArgumentException for consistency across the codebase, or document why a
different exception type is appropriate here.
```suggestion
throw new RuntimeException("GAR_TEST_DATA not found")
```
##########
maven-projects/spark/graphar/src/test/scala/org/apache/graphar/BaseTestSuite.scala:
##########
@@ -29,10 +29,25 @@ abstract class BaseTestSuite extends AnyFunSuite with
BeforeAndAfterAll {
var spark: SparkSession = _
override def beforeAll(): Unit = {
- if (System.getenv("GAR_TEST_DATA") == null) {
- throw new IllegalArgumentException("GAR_TEST_DATA is not set")
+ def resolveTestData(): String = {
+ Option(System.getenv("GAR_TEST_DATA"))
+ .orElse(Option(System.getProperty("gar.test.data")))
+ .getOrElse {
+ val candidates = Seq("../../testing", "../testing", "testing")
+ candidates
+ .map(p => new java.io.File(p).getAbsoluteFile)
+ .find(d =>
+ new java.io.File(d, "ldbc_sample/csv/ldbc_sample.graph.yml")
+ .exists()
+ )
+ .map(_.getAbsolutePath)
+ .getOrElse(
+ throw new IllegalArgumentException("GAR_TEST_DATA not found")
Review Comment:
The error message is less informative than the one used in TestUtil.java.
TestUtil.java provides a more detailed message that explains both the
environment variable option and the directory structure requirement. Consider
updating the message to match the clarity of TestUtil.java's error message
which states: "GAR_TEST_DATA not found or invalid. Please set GAR_TEST_DATA
environment variable to point to the testing directory or ensure the testing
directory exists with ldbc_sample/csv/ldbc_sample.graph.yml".
```suggestion
throw new IllegalArgumentException(
"GAR_TEST_DATA not found or invalid. Please set
GAR_TEST_DATA environment variable to point to the testing directory or ensure
the testing directory exists with ldbc_sample/csv/ldbc_sample.graph.yml"
)
```
##########
maven-projects/java/src/test/java/org/apache/graphar/graphinfo/GraphInfoTest.java:
##########
@@ -32,7 +32,25 @@
import org.junit.Test;
public class GraphInfoTest {
- public static final String root = System.getenv("GAR_TEST_DATA") + "/java";
+ private static String resolveTestData() {
+ String path = System.getenv("GAR_TEST_DATA");
+ if (path == null) {
+ path = System.getProperty("gar.test.data");
+ }
+ if (path == null) {
+ String[] candidates = {"../../testing", "../testing", "testing"};
+ for (String p : candidates) {
+ java.io.File dir = new java.io.File(p).getAbsoluteFile();
+ if (new java.io.File(dir,
"ldbc_sample/csv/ldbc_sample.graph.yml").exists()) {
+ return dir.getAbsolutePath();
+ }
+ }
+ throw new IllegalStateException("GAR_TEST_DATA not found");
+ }
+ return path;
Review Comment:
The validation approach differs from TestUtil.java which performs additional
verification after finding a path. TestUtil.java re-validates the chosen path
to ensure both the directory and the marker file exist. This double-checking
approach is more robust and catches edge cases. Consider adding a final
validation step after selecting a path to ensure it's valid before returning it.
```suggestion
path = dir.getAbsolutePath();
break;
}
}
if (path == null) {
throw new IllegalStateException("GAR_TEST_DATA not found");
}
}
java.io.File baseDir = new java.io.File(path).getAbsoluteFile();
java.io.File markerFile =
new java.io.File(baseDir,
"ldbc_sample/csv/ldbc_sample.graph.yml");
if (!baseDir.isDirectory() || !markerFile.exists()) {
throw new IllegalStateException("GAR_TEST_DATA not found");
}
return baseDir.getAbsolutePath();
```
##########
maven-projects/java/src/test/java/org/apache/graphar/graphinfo/GraphInfoTest.java:
##########
@@ -32,7 +32,25 @@
import org.junit.Test;
public class GraphInfoTest {
- public static final String root = System.getenv("GAR_TEST_DATA") + "/java";
+ private static String resolveTestData() {
+ String path = System.getenv("GAR_TEST_DATA");
+ if (path == null) {
+ path = System.getProperty("gar.test.data");
+ }
+ if (path == null) {
+ String[] candidates = {"../../testing", "../testing", "testing"};
+ for (String p : candidates) {
+ java.io.File dir = new java.io.File(p).getAbsoluteFile();
+ if (new java.io.File(dir,
"ldbc_sample/csv/ldbc_sample.graph.yml").exists()) {
+ return dir.getAbsolutePath();
+ }
+ }
+ throw new IllegalStateException("GAR_TEST_DATA not found");
Review Comment:
The exception type differs from the one used in TestUtil.java, which uses
RuntimeException. Consider using RuntimeException instead of
IllegalStateException for consistency across the codebase, or document why a
different exception type is appropriate here.
```suggestion
throw new RuntimeException("GAR_TEST_DATA not found");
```
##########
maven-projects/spark/graphar/src/test/scala/org/apache/graphar/BaseTestSuite.scala:
##########
@@ -29,10 +29,25 @@ abstract class BaseTestSuite extends AnyFunSuite with
BeforeAndAfterAll {
var spark: SparkSession = _
override def beforeAll(): Unit = {
- if (System.getenv("GAR_TEST_DATA") == null) {
- throw new IllegalArgumentException("GAR_TEST_DATA is not set")
+ def resolveTestData(): String = {
+ Option(System.getenv("GAR_TEST_DATA"))
+ .orElse(Option(System.getProperty("gar.test.data")))
+ .getOrElse {
+ val candidates = Seq("../../testing", "../testing", "testing")
+ candidates
+ .map(p => new java.io.File(p).getAbsoluteFile)
+ .find(d =>
+ new java.io.File(d, "ldbc_sample/csv/ldbc_sample.graph.yml")
+ .exists()
+ )
+ .map(_.getAbsolutePath)
+ .getOrElse(
+ throw new IllegalArgumentException("GAR_TEST_DATA not found")
+ )
+ }
}
- testData = System.getenv("GAR_TEST_DATA")
+
+ testData = resolveTestData()
Review Comment:
The validation approach differs from TestUtil.java which performs additional
verification after finding a path. TestUtil.java re-validates the chosen path
to ensure both the directory and the marker file exist. This double-checking
approach is more robust and catches edge cases. Consider adding a final
validation step after selecting a path to ensure it's valid before returning it.
##########
maven-projects/spark/graphar/src/test/scala/org/apache/graphar/BaseTestSuite.scala:
##########
@@ -29,10 +29,25 @@ abstract class BaseTestSuite extends AnyFunSuite with
BeforeAndAfterAll {
var spark: SparkSession = _
override def beforeAll(): Unit = {
- if (System.getenv("GAR_TEST_DATA") == null) {
- throw new IllegalArgumentException("GAR_TEST_DATA is not set")
+ def resolveTestData(): String = {
+ Option(System.getenv("GAR_TEST_DATA"))
+ .orElse(Option(System.getProperty("gar.test.data")))
+ .getOrElse {
+ val candidates = Seq("../../testing", "../testing", "testing")
+ candidates
+ .map(p => new java.io.File(p).getAbsoluteFile)
+ .find(d =>
+ new java.io.File(d, "ldbc_sample/csv/ldbc_sample.graph.yml")
+ .exists()
Review Comment:
The validation logic is inconsistent with the existing implementation in
TestUtil.java. The Scala code only checks if the file exists, but TestUtil.java
also verifies that the directory exists and is actually a directory. This could
lead to false positives if the parent path doesn't exist. Consider adding an
additional check to verify that the parent directory exists and is a directory
before checking for the specific file.
```suggestion
d.exists() && d.isDirectory &&
new java.io.File(d, "ldbc_sample/csv/ldbc_sample.graph.yml")
.isFile
```
##########
maven-projects/java/src/test/java/org/apache/graphar/graphinfo/GraphInfoTest.java:
##########
@@ -32,7 +32,25 @@
import org.junit.Test;
public class GraphInfoTest {
- public static final String root = System.getenv("GAR_TEST_DATA") + "/java";
+ private static String resolveTestData() {
+ String path = System.getenv("GAR_TEST_DATA");
+ if (path == null) {
+ path = System.getProperty("gar.test.data");
+ }
+ if (path == null) {
+ String[] candidates = {"../../testing", "../testing", "testing"};
+ for (String p : candidates) {
+ java.io.File dir = new java.io.File(p).getAbsoluteFile();
+ if (new java.io.File(dir,
"ldbc_sample/csv/ldbc_sample.graph.yml").exists()) {
+ return dir.getAbsolutePath();
Review Comment:
The validation logic is inconsistent with the existing implementation in
TestUtil.java. The current code only checks if the file exists, but
TestUtil.java also verifies that the directory exists and is actually a
directory. This could lead to false positives if the parent path doesn't exist.
Consider adding an additional check to verify that the parent directory exists
and is a directory before checking for the specific file.
```suggestion
if (dir.exists() && dir.isDirectory()) {
java.io.File graphFile =
new java.io.File(dir,
"ldbc_sample/csv/ldbc_sample.graph.yml");
if (graphFile.exists()) {
return dir.getAbsolutePath();
}
```
--
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]
